lint's strict bool mode (was: Re: CVS commit: src/usr.sbin/flashctl)

[Roland Illig <roland.illig%gmx.de@localhost>]

Am 13.05.2024 um 16:06 schrieb Taylor R Campbell:
>> Module Name:    src
>> Committed By:   rillig
>> Date:           Sun May 12 19:03:55 UTC 2024
>>
>> Modified Files:
>>         src/usr.sbin/flashctl: flashctl.c
>>
>> Log Message:
>> flashctl: fix lint's strict bool mode with Clang preprocessor
>>
>> Treating the return value from the <ctype.h> character classification
>> functions as an 'int' is neither elegant nor idiomatic, but it works for
>> now.
>>
>> -               if (!isxdigit((unsigned char)str[2]))
>> +               if (isxdigit((unsigned char)str[2]) == 0)
>
> Why is this change necessary?  Weren't you teaching lint to handle
> this case without complaining?

Yes, I did. The thing is that I only ever tested lint's strict bool mode
with the GCC preprocessor, and that preprocessor marks every token from
a macro expansion as coming from a system header or coming from the
user's code. Therefore, lint could be more lenient when checking the
result of the isxdigit macro, as its main operator, the cast to '(int)',
is from a system header, so it's OK that it has the "wrong" type.

On the other hand, the strcmp function is not listed in "namespace.h"
and does not have a macro definition, and although the function
declaration comes from a system header, the GCC preprocessor does not
mark the function call expression as coming from a system header.

This is the difference by which lint treats 'isxdigit(ch)' as bool-like
but 'strcmp(s1, s2)' as strictly 'int'.

A few days or weeks ago, Christos started builds that have both
MKLLVM=yes and MKLINT=yes, thus using the Clang preprocessor as lint's
preprocessor. Apparently nobody else has done this in the last few
years. Clang's preprocessor's output does not mark the tokens from macro
expansions as coming from a system header or from the user's code. Due
to this, lint can no longer be lenient for system header macro
expansions as it doesn't get the system header information anymore.

> We shouldn't change anything like
>
> 	if (!isxdigit(...))
> 	if (ferror(...))
>
> to
>
> 	if (isxdigit(...) == 0)
> 	if (ferror(...) != 0)
>
> The original is clearer and idiomatic code, even if it's a little
> silly that the return value is declared as int and not bool
> (presumably for historical reasons, if the interfaces were defined
> before bool existed).

I agree. For usr.bin/error, I tried a different variant, namely to only
activate lint's strict bool mode when MKLLVM is undefined, thus when the
GCC preprocessor is used. I guess activating the strict bool mode only
in GCC mode is good enough to catch all type mismatches.

The combination of MKLLVM=yes MKLINT=yes is also the cause why lint now
allows do-while-0 even in strict bool mode, as the FD_ZERO macro uses
that idiom and I didn't dare to change the macro to do-while-false, as
that would either require <sys/stdbool.h>, or to deviate from the idiom
by using 'do { ... } while (0 != 0)', as that would look suspicious.

I will switch usr.sbin/flashctl to the only-in-GCC-mode variant and
restore the previous idiomatic code.

Roland