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@xxxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxx> >>>> --- >>>> 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
