Re: [PATCH] src/sort.c: pipe_fork is always called with tries > 0

[Jim Meyering]

Kovarththanan Rajaratnam wrote:
> Jim Meyering wrote:
>> Kovarththanan Rajaratnam wrote:
>>> clang detected the following false positive:
>>>
>>> sort.c:906:11: warning: The left operand of '<' is a garbage value
>>>   if (pid < 0)
>>>       ~~~ ^
>>>
>>> src/sort.c: Add an assert indicating that pipe_fork is always called with 
>>> tries > 0. This allows clang to deduce that 'pid' will always be set/is 
>>> valid.
>>
>> Thank you for all of the patches.
>> I too would like to address all of clang's warnings about
>> coreutils, but it seems like clang is not quite mature enough
>> for me to be comfortable adding the many assertions required
>> to quiet its false positive warnings.
>
> Sadly, clang doesn't yet support interprocedural analysis, so it doesn't
> reason about how the functions are called/used (which would have avoided
> this particular false positive).
>
>> Is there any other way to make clang understand that no warning is
>> required here?
>
> Sadly, no. It sounds as though you might be open to annotations (similar
> to splint?) or am I getting the wrong message here?

I would rather avoid using annotations, since I don't see
how that can scale to multiple tools.

However, one compromise might be to use a new name,
say foo_assert(expr), that would be defined to assert(expr)
when "lint" is defined, and otherwise to (void)0.

At first I though to use clang_assert, but obviously we
want a more generic name, since it will probably be useful also
with coverity.

I'm sure someone can come up with a good name.
Then, with definitions in system.h, alongside those for IF_LINT,
I'd probably accept your patches.  Though please do read the
HACKING guidelines.  One thing I noticed in your patches is that
you omitted the space that we always put just before a function
invocation's opening parenthesis.

One twist with this proposal is that we'd have to adjust
the syntax-check rule that determines whether assert.h is used.