Re: [PATCH 10/26] Check for EOF while parsing mails

[Jeff King <peff@peff.net>]

On Fri, Apr 28, 2017 at 03:33:41PM +0200, Johannes Schindelin wrote:

> As usual, EOF is defined as -1 in Git for Windows' context, meaning that
> we look at the last entry of the sane_ctype array, which returns 0 for any
> sane_istest(x,mask) test for x >= 0x80:
> 
>         /* Nothing in the 128.. range */
> 
> So it would appear that it happens to work, but I doubt that it was
> intentional.

Yeah, that was the same analysis I came to. Even though it works, it is
a potential portability problem if a platform defines EOF in a weird
way. The "right" thing in such a case is probably checking explicitly
for EOF, but I'd hate to add an extra conditional to sane_istest(). It's
a fairly hot code path. So I'm fine with punting on that until we see
evidence of such a system.

> Having said that, it is really curious why Coverity should get confused by
> the code and not realize that casting a negative number to (unsigned char)
> will make it valid as an index for the sane_ctype array.

Yeah, that is the part that confuses me, too. It seems like an easy
case to get right.

Oh well. If you are tweaking it to handle EOF separately for other
reasons, then the Coverity question goes away entirely.

-Peff