On Mon, 6 Jun 2022 06:50:44 GMT, Erik Joelsson <er...@openjdk.org> wrote:
>> Leo Korinth has updated the pull request incrementally with one additional >> commit since the last revision: >> >> wip > > make/RunTests.gmk line 39: > >> 37: >> ################################################################################ >> 38: >> 39: HASH:=\# > > HASH is already defined in MakeBase.gmk so should be available here. thanks, in the new version I do not even use it. > make/RunTests.gmk line 47: > >> 45: define IfPrepend >> 46: $(if $(strip $1),$(strip $2)$(strip $1),) >> 47: endef > > These two probably fits better in make/common/Utils.gmk. > > Also please have a look at the [Code Conventions for the Build > System](http://openjdk.java.net/groups/build/doc/code-conventions.html). One > liner macros like this we like to format like this: > > > IfPrepend = \ > $(if $(strip $1),$(strip $2)$(strip $1),) I moved them both to `make/common/Utils.gmk`. > make/RunTests.gmk line 51: > >> 49: define TestID >> 50: $(subst .java,,$(subst .java$(HASH),,$(suffix $(notdir $1)))) >> 51: endef > > This macro deserves a comment explaining what it's trying to do. I added a description and made it a one-liner as well > make/RunTests.gmk line 53: > >> 51: endef >> 52: >> 53: define HashTestID > > This one too. I removed HashTestID and added the functionality directly into TestID > make/RunTests.gmk line 377: > >> 375: # either absolute or relative to any of the TEST_BASEDIRS or test roots. >> 376: define ParseJtregTestSelection >> 377: $(call IfAppend, \ > > Please adjust indentation of the whole body here. Fixed > make/RunTests.gmk line 448: > >> 446: # Now intelligently convert the test selection given by the user in TEST >> 447: # into a list of fully qualified test descriptors of the tests to run. >> 448: TESTS_TO_RUN:=$(strip $(foreach test,$(TEST),$(call >> ParseTestSelection,$(test)))) > > I think this rewrite is more elegant, but logically there is a difference as > it no longer breaks early. I would like to have Magnus' opinion on this as he > wrote this originally. > > Style wise, please leave spaces around operators and space after comma when > possible. The top level strip should handle any whitespace issues caused by > this. I changed the code to use `$(or)` instead of `$(foreach)`, and I added spaces. ------------- PR: https://git.openjdk.java.net/jdk/pull/9028
