On Wed, Oct 26 2022, Junio C Hamano wrote: > Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> writes: > >> Define the variables that make up TEST_OBJS earlier, and don't go back >> & forth in their definition. Before we'd first append $X to >> $(TEST_PROGRAMS), and then substitute $X back out of it to define >> $(TEST_OBJS). Let's instead add a new $(TEST_PROGRAM_OBJS) variable, >> which avoids this needless back & forth substitution. > > Makes sense, I guess. So TEST_OBJS is no longer used? Yes, sorry I'll clarify that in a re-roll. >> TEST_PROGRAMS = $(patsubst %,t/helper/%$X,$(TEST_PROGRAMS_NEED_X)) >> +all:: $(TEST_PROGRAMS) > > This change is not necessary to achieve the stated goal of this > step, though. It is one of those "while at it" distraction that > consumes our already constrained reviewer bandwidth, no? I figured this would be better use of that bandwith, since the reviewer doesn't need to wonder why these are still spread befo/after the main body of the change. Not everyone is keenly aware of the at first odd way a Makefile is read (per "3.7 How 'make' Reads a Makefile" in the GNU make manual). But I'm happy to eject this part if that helps, but... > Having said that, "all::" being able to be built up with independent > pieces shine here in this split from the original. It probably is > easier to reason about while seeing this isolated area of Makefile > what is being done to TEST_PROGRAMS. ...here I'm not quite sure if you want to keep it after all or nat... > The rest of the patch is quite straight-forward renaming of > TEST_OBJS to TEST_PROGRAM_OBJS and an improvement of how the > elements on the list are computed from the source-of-truth list that > is TEST_PROGRAMS_NEED_X that looks correct. Thanks for the quick review!
