On 01/02/2019 21:46, Junio C Hamano wrote:
> Ramsay Jones <ramsay@xxxxxxxxxxxxxxxxxxxx> writes:
>
>> In order to enable greater user customisation of the SPARSE_FLAGS
>> variable, we introduce a new SP_EXTRA_FLAGS variable to use for
>> target specific settings. Without using the new variable, setting
>> the SPARSE_FLAGS on the 'make' command-line would also override the
>> value set by the target-specific rules in the Makefile (effectively
>> making them useless). In addition, we initialise the SPARSE_FLAGS
>> to the default (empty) value using a conditional assignment (?=).
>> This allows the SPARSE_FLAGS to be set from the environment as
>> well as from the command-line.
>
> Thanks for a detailed and clear explanation here and in the cover
> letter. I agree with the motivation and most of the things I see in
> this patch, but one thing that stands out at me is if we still want
> to += append to SP_EXTRA_FLAGS in target specific way. Before this
> patch, because SPARSE_FLAGS was a dual use variable, it needed +=
> appending to it in these two places, but that rationale is gone with
> this patch.
As Luc surmised, in his reply, my intention was that SP_EXTRA_FLAGS
should be used for any 'internal' settings (not just the target
specific settings), whereas SPARSE_FLAGS would now be used _only_ for
user customisation.
The commit message doesn't make that clear, (and the patch text adds
to the confusion, since only target specific settings are changed) so
I need to reword that somehow. Also, ...
> Also, don't we want to clear SP_EXTRA_FLAGS at the beginning?
... (Ahem) I just simply forgot to initialise the new variable! :(
(Yes, it actually doesn't matter, but it gives a wrong impression). ;-)
BTW, the first name I chose was SP_FLAGS, but while editing the second
hunk I decided that wasn't a good name. On several other projects I have
seen exactly this 'split' happen, where the user facing variable was
called <something>_FLAGS and the 'internal' variable was then called
<something>_EXTRA_FLAGS, so I decided to go with that instead. (Yes, I
abbreviated SPARSE). However, I have to say that I have also seen (less
often) the exact opposite: "... if some idiot user wants to add extra
flags ...". :-D
So, yes SP_EXTRA_FLAGS could be used for other 'internal' uses; for
example, look back to commit 6bc8606be3 ("config.mak.uname: remove
SPARSE_FLAGS setting for cygwin", 2018-02-12), which removed:
'SPARSE_FLAGS = -isystem /usr/include/w32api -Wno-one-bit-signed-bitfield'
from config.mak.uname. As you can see, although gcc could find the
win32 header files, sparse needed a little help. Also, the win32 system
header files had an instance of a 'one-bit signed bitfield', which caused
sparse to spew many many many errors. If I needed to do something like
that again, then I would use SP_EXTRA_FLAGS instead.
[Looking back now, I am a little shocked that it seems to have taken
me nearly 5 years to submit that patch! :-P ]
I could give quite a few examples, but ... Oh wait! ... Hmm, it seems
that I need to add a new patch to remove line 558 of config.mak.uname.
This line has a setting for SPARSE_FLAGS in the MINGW section of that
file. Back in around 2011, having ported sparse to MinGW (the original
msysgit, not MSYS2), I naturally had the same issue with the Win32
header files. Since I didn't upstream my sparse patches, I don't think
anyone can be running sparse on MinGW these days.
Anyway, its late, so I will look at redoing the patches soon.
ATB,
Ramsay Jones
