Hi Please see the comments below.
BR imran On Mon, Nov 24, 2014 at 8:58 PM, Bill Spitzak <spit...@gmail.com> wrote: > > > On 11/24/2014 05:08 AM, Imran Zaman wrote: >> >> Thanks Bill for your comments. Please see my comments inline. >> >> On Fri, Nov 21, 2014 at 9:31 PM, Bill Spitzak <spit...@gmail.com> wrote: >>> >>> There is no need to pass endptr, because your functions will always fail >>> if >>> it is not set to point to the terminating null. >>> >> [IZ] endptr is passed other than null; please see the whole patch. why >> do u think it will fail? > > > What I meant was that if this function returns success, endptr will ALWAYS > be set to the string+strlen(string), ie at the NULL. As the caller can > easily create this answer itself it seems useless to pass a pointer to store > it. > > Although endptr might not point at the null when this fails, it seems hard > for the caller to do anything useful with this. It would have to first > determine if the error was due to something other than trailing characters, > which would involve replicating the entire function locally. > > Therefore I think this argument is useless and there is no reason to support > it. > > Also the grep below shows that nobody uses the endptr except to do the error > checking that this function is removing the need for. > [IZ2] This is the case where endptr is used in patch. I can remove endptr but it seems its used so i have to keep the existing functionality in place. + if (!weston_strtoi(pid, &end, 0, &other) || end != pid + 10) >>> I also suspect that the base is never used. In addition a possible future >>> fix would be to accept 0x for hex but a leading zero is decimal, not >>> octal, >>> which means this function would generate the base. >>> >> [IZ] Base is used in weston code as well, please see the whole patch. > > > Out of 14 calls (found with "git grep strto"), 6 use 10, 1 uses 16, and 7 > use 0. > > I believe all the "10"s are to avoid the unexpected-octal problem. I can't > be absolutely certain, but I suspect the reason this is done is to prevent > unexpected octal conversion, and in fact there is no desire to make 0x > prefix not work. > > This is a good indication that perhaps your functions should do this as > well. Basically only base 10 and 0x for base 16 work. > > wscreensaver-glue uses 16 to read the color map out of an xpm file. I cannot > find any code in weston that calls this xpm converter. > >>> I am also worried about passing the out ptr. Depending on the sizes of >>> int, >>> long, and long long this will easily result in code that compiles on one >>> platform but not on another. Returning the value would avoid this and >>> reduce >>> the number of functions. Instead pass a pointer to a variable to store >>> the >>> error into, or use errno? >>> >>> Would prefer you not test the addresses of out parameters for null. It >>> should crash at this point, as this is a programming error, not a runtime >>> error. Returning false will make it hard to figure out what is wrong. >>> >> [IZ] can u please elaborate it bit more as I can add more test cases >> to cover the corner case if I understand clearly what do you want to >> highlight? > > > I want it it CRASH if null is passed for the output pointer. The easiest way > to do this is to not check if it is null. > > Please do not return zero or do anything other than crash right at that > point. This is not an environment or input error, passing null for this is a > programming error that can be fixed at compile time. Hiding the error so it > is not obvious the first time the program is run is completely > non-productive. > [IZ2] Sorry still not able to grasp what you want to highlight. Can you please give an example? >>> It is useful to pass -1 for strtoul and this should continue to work. It >>> is >>> the only easily-recognized value for "largest number possible". May be ok >>> to >>> not allow other negative numbers however. > > > Yes I don't think preserving -1 is a big deal, but I have used this in the > past. The caller can certainly check for this string before calling the > converter. BR imran _______________________________________________ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
