Max Kirillov <m...@max630.net> writes: > On Tue, Sep 15, 2015 at 08:50:03AM +0200, Matthieu Moy wrote: >> --- a/git-compat-util.h >> +++ b/git-compat-util.h >> @@ -814,6 +814,9 @@ static inline int strtoul_ui(char const *s, int base, >> unsigned int *result) >> char *p; >> >> errno = 0; >> + /* negative values would be accepted by strtoul */ >> + if (strchr(s, '-')) >> + return -1; >> ul = strtoul(s, &p, base); >> if (errno || *p || p == s || (unsigned int) ul != ul) >> return -1; >> >> What do you think? > > Explicit rejection of '-' is of course useful addition. > > I still find "(unsigned int) ul != ul" bad. As far as I > understand it makes no sense for i386.
Nothing would make sense here for i386: there's no case where you want to reject anything on this architecture. Well, you may have expected strtoul to reject big numbers, but it did not and it's too late. > And even for 64-bit it's too obscure. In form of "(ul & 0xffffffffL) > == 0" it would be more clear. I disagree. "(unsigned int) ul != ul" reads immediately as "if casting ul to unsigned int changes its value", regardless of sizeof(int). This is exactly what the check is doing. > Or just make explicit comparison with intended limit, like I did. What you really want is to compare with UINT_MAX which would not make sense on 32 bits architectures. -- Matthieu Moy http://www-verimag.imag.fr/~moy/ -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
