Re: [PATCH] CLEANUP: h2: Help static analyzers understand the list's end marker

2020-03-19 Thread Tim Düsterhus
Willy,

Am 19.03.20 um 15:55 schrieb Willy Tarreau:
> Actually I'm pretty sure that I did it this way precisely for performance
> reasons: avoid repeatedly checking a pointer for half of the headers which
> are pseudo headers (method, scheme, authority, path just for the request).
> 
> It's perfectly possible that the difference is negligible though, but if
> it's not, I'm sorry but I'll favor performance over static analysers'
> own pleasure. So this one will definitely deserve a test.

Yes, the performance <-> static analyzer trade-off not preferring the
static analyzers is acceptable to me.

Best regards
Tim Düsterhus



Re: [PATCH] CLEANUP: h2: Help static analyzers understand the list's end marker

2020-03-19 Thread Willy Tarreau
Hi Tim,

On Thu, Mar 19, 2020 at 03:15:24PM +0100, Tim Duesterhus wrote:
> Willy,
> 
> I know you dislike adjusting code to please static analyzers, but I'd argue
> that using the new IST_NULL + isttest() combination is easier to understand 
> for humans as well. A simple .ptr == NULL check might also be slightly faster
> compared to isteq() with an empty string?
> 
> I have verified that reg-tests pass, but as this is deep within the internals
> please check this carefully.
> 
> Best regards
> Tim Düsterhus
> 
> Apply with `git am --scissors` to automatically cut the commit message.
> 
> -- >8 --
> Clang Static Analyzer (scan-build) was having a hard time to understand that
> `hpack_encode_header` would never be called with a garbage value for `v`.
> 
> It failed to detect that `if (isteq(n, ist(""))) break;` would exist the
> loop in all cases. By setting `n` to `IST_NULL` and checking with
> `isttest()` it no longer complains.

Actually I'm pretty sure that I did it this way precisely for performance
reasons: avoid repeatedly checking a pointer for half of the headers which
are pseudo headers (method, scheme, authority, path just for the request).

It's perfectly possible that the difference is negligible though, but if
it's not, I'm sorry but I'll favor performance over static analysers'
own pleasure. So this one will definitely deserve a test.

Thanks,
Willy