Re: [PATCH] read-cache.c: fix a sparse warning

[Ramsay Jones <ramsay@ramsayjones.plus.com>]

On 17/09/18 17:27, Ramsay Jones wrote:
> 
> 
> On 17/09/18 15:15, Ben Peart wrote:
>>
>>
>> On 9/16/2018 3:17 AM, Eric Sunshine wrote:
>>> On Fri, Sep 14, 2018 at 7:29 PM Ramsay Jones
>>> <ramsay@ramsayjones.plus.com> wrote:
>>>> At one time, the POSIX standard required the type used to represent
>>>> a thread handle (pthread_t) be an arithmetic type. This is no longer
>>>> the case, probably because different platforms used to regularly
>>>> ignore that requirement.  For example, on cygwin a pthread_t is a
>>>> pointer to a structure (a quite common choice), whereas on Linux it
>>>> is defined as an 'unsigned long int'.
>>>>
>>>> On cygwin, but not on Linux, 'sparse' currently complains about an
>>>> initialiser used on a 'struct load_index_extensions' variable, whose
>>>> first field may be a pthread handle (if not compiled with NO_PTHREADS
>>>> set).
>>>>
>>>> In order to fix the warning, move the (conditional) pthread field to
>>>> the end of the struct and change the initialiser to use a NULL, since
>>>> the new (unconditional) first field is a pointer type.
>>>>
>>>> Signed-off-by: Ramsay Jones <ramsay@ramsayjones.plus.com>
>>>> ---
>>>> If you need to re-roll your 'bp/read-cache-parallel' branch, could you
>>>> please squash this into the relevant patch (commit a090af334,
>>>> "read-cache: load cache extensions on a worker thread", 2018-09-12).
>>>
>>> The information contained in this commit message is so useful that it
>>> might make sense to plop this patch at the end of the series rather
>>> than merely squashing it in. (Or, if it is squashed, include the above
>>> explanation in the commit message of the appropriate patch.)
>>>
>>
>> I'm happy to squash it in if I end up re-rolling the patch series.  I'll include the information in the commit message above as a comment so that it is in close proximity to the code impacted.
>>
> 
> I will be happy with whatever decision you take regarding whether
> to squash this in or add it on top of your series. However, if you
> do squash it in, please don't add the commit message info as a
> comment to the code. No matter how you word it, I can't imagine
> that it would be anything but superfluous - the kind of comment
> that would be removed after review! ;-)
> 
> The information in the commit message about pthread_t, which I
> thought was common knowledge, was not really the main point of
> the argument supporting the patch. (Search for "How do I print
> a pthread_t", for variations on this theme).
> 
> The main point for me: don't conditionally include a field at the
> beginning of a structure and then use an initialiser in a variable
> declaration. (Unless, I suppose, the first unconditional field had
> the same type - but probably not not even then!)
> 
> The fact that the conditionally included field itself had an 'opaque'
> type was just an additional complication.

BTW, I just noticed that you explicitly initialise each field of
that structure (not surprising), so an even simpler solution is
to simply remove the unneeded initialiser! ;-)

ATB,
Ramsay Jones