Ævar Arnfjörð Bjarmason [off-list ref] writes:
quoted
quoted
-PERL_DEFINES = $(PERL_PATH_SQ):$(PERLLIB_EXTRA_SQ):$(perllibdir_SQ)
-
-PERL_DEFINES := $(PERL_PATH_SQ) $(PERLLIB_EXTRA_SQ) $(perllibdir_SQ)
+PERL_DEFINES :=
+PERL_DEFINES += $(PERL_PATH_SQ)
+PERL_DEFINES += $(PERLLIB_EXTRA_SQ)
+PERL_DEFINES += $(perllibdir_SQ)
PERL_DEFINES += $(RUNTIME_PREFIX)
I don't think we generally use simply-expanded variables in our Makefile
unless there's a reason. Do we actually need it here? Obviously not new
in your patch, but just a curiosity I noticed while reading it.
I didn't notice it at the time. I suppose it could be changed to not do
expansion, but per-se unrelated to the more narrorw bugfix in this
patch.
Actually, strictly speaking there was *no* bug because assigning
three items with := made sure the previous recursively expanded one
to be ineffective. In other words, there was a valid reason to use
":=" there in the original version.
Now your patch removed the recursively expanded one that was
immediately invalidated, there no longer is a reason to use :=
there. So "unrelated to the more narrow bugfix" is a rather lame
excuse to do only half a task. If we remove that extra one (which
is a good thing), then we should correct := into = because the
original used := only because there was the unwanted extra one, no?
On Thu, May 06 2021, Junio C Hamano wrote:
Ævar Arnfjörð Bjarmason [off-list ref] writes:
quoted
quoted
quoted
-PERL_DEFINES = $(PERL_PATH_SQ):$(PERLLIB_EXTRA_SQ):$(perllibdir_SQ)
-
-PERL_DEFINES := $(PERL_PATH_SQ) $(PERLLIB_EXTRA_SQ) $(perllibdir_SQ)
+PERL_DEFINES :=
+PERL_DEFINES += $(PERL_PATH_SQ)
+PERL_DEFINES += $(PERLLIB_EXTRA_SQ)
+PERL_DEFINES += $(perllibdir_SQ)
PERL_DEFINES += $(RUNTIME_PREFIX)
I don't think we generally use simply-expanded variables in our Makefile
unless there's a reason. Do we actually need it here? Obviously not new
in your patch, but just a curiosity I noticed while reading it.
I didn't notice it at the time. I suppose it could be changed to not do
expansion, but per-se unrelated to the more narrorw bugfix in this
patch.
Actually, strictly speaking there was *no* bug because assigning
three items with := made sure the previous recursively expanded one
to be ineffective. In other words, there was a valid reason to use
":=" there in the original version.
Yes, there wasn't any bug with the the eventual value being
incorrect. I.e. both of these are equivalent in a Makefile:
FOO = abc
FOO := def
FOO += ghi
And:
FOO = abc
FOO = def
FOO += ghi
Both will yield "def ghi". They're just different in a case like:
X = Y
FOO = abc
FOO := $(X)
X = Z
FOO += ghi
Where using := will echo "Y ghi", and using = will echo "Z ghi". As a
practical matter the distinction doesn't matter in this case.
Now your patch removed the recursively expanded one that was
immediately invalidated, there no longer is a reason to use :=
there. So "unrelated to the more narrow bugfix" is a rather lame
excuse to do only half a task. If we remove that extra one (which
is a good thing), then we should correct := into = because the
original used := only because there was the unwanted extra one, no?
I don't see how removing the stray line changes the reason to use ":="
or "=" there. I agree it should be removed, it's just unrelated to
removing the stay line. Looking at 07d90eadb50 it's clear that it's just
some copy/pasting error.
Maybe the confusion is that I'm using "bug" closer to a meaning of "a
thing nobody intended to be in the program", not just "a
behavior-changing issue observable from the outside".
In any case. I can just submit a patch on top of this in a v2. I
continue to find it hard to discover the line between superfluous
while-we're-at-it fixes in your mind v.s. "we should fix this while
we're at it" though :)
But regarding the "half a task" it seems to me that these are different
issues; I don't think that's a point worth arguing in this case
specifically (let's just fix it, and I will), but perhaps I'm missing
something subtle with regards to Makefile semantics per my examples
above so it really is all one issue, and I'd like to understand how
they're entwined.
On Thu, May 06, 2021 at 11:04:34AM +0200, Ævar Arnfjörð Bjarmason wrote:
quoted
Actually, strictly speaking there was *no* bug because assigning
three items with := made sure the previous recursively expanded one
to be ineffective. In other words, there was a valid reason to use
":=" there in the original version.
Yes, there wasn't any bug with the the eventual value being
incorrect. I.e. both of these are equivalent in a Makefile:
FOO = abc
FOO := def
FOO += ghi
And:
FOO = abc
FOO = def
FOO += ghi
Both will yield "def ghi". They're just different in a case like:
X = Y
FOO = abc
FOO := $(X)
X = Z
FOO += ghi
Where using := will echo "Y ghi", and using = will echo "Z ghi". As a
practical matter the distinction doesn't matter in this case.
Yeah, I don't think the ":=" was impacting the bug or no bug (not to
mention that even if we duplicated those entries in the variable, it
_still_ wouldn't be a bug, since the whole point of the variable is just
to notice when the content changes).
quoted
Now your patch removed the recursively expanded one that was
immediately invalidated, there no longer is a reason to use :=
there. So "unrelated to the more narrow bugfix" is a rather lame
excuse to do only half a task. If we remove that extra one (which
is a good thing), then we should correct := into = because the
original used := only because there was the unwanted extra one, no?
I don't see how removing the stray line changes the reason to use ":="
or "=" there. I agree it should be removed, it's just unrelated to
removing the stay line. Looking at 07d90eadb50 it's clear that it's just
some copy/pasting error.
Yeah, I'd agree it is truly orthogonal. I don't mind seeing it cleaned
up in addition (or am even actively happy to see it cleaned up :) ), but
IMHO it would not need to hold up the series.
-Peff