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.
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.
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.
_______________________________________________
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel
