[PATCH] D101597: [test-suite] New SPEC2017 macro and enhancements to run_specpp macro

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur added inline comments.


================
Comment at: External/SPEC/SpecCPU2017.cmake:371
 macro(speccpu2017_run_specpp)
-  cmake_parse_arguments(_arg "" "" "SPECPP_SRCS;SPECPP_DEFS" ${ARGN})
+  cmake_parse_arguments(_arg "" "SPECPP_TARGET" "SPECPP_SRCS;SPECPP_DEFS" ${ARGN})
   set(_specpp_bin ${TEST_SUITE_SPEC2017_ROOT}/bin/specpp)
----------------
naromero77 wrote:
> Meinersbur wrote:
> > [suggestion] Argument could be more self-explanatory or documented. Suggestion: `ADD_SOURCES_TO_TARGET`
> > 
> > (The `SPECPP_` prefixes seem unnecessary, these are function arguments that don't clash with any namespace)
> Would this work?
> 
> ```
> cmake_parse_arguments(_arg "" "TARGET" "ADD_SRCS_TO_TARGET;DEFS" ${ARGN})
> ```
> 
`ADD_SRCS_TO_TARGET` doesn't sound like an option to list source files. `SRCS_TO_BE_PROCESSED_AND_ADDED_TO_TARGET` would match better, but unreasonable long.

Maybe just `TARGET` and `SRCS` and add to the description of speccpu2017_run_specpp that the command adds the result from specpp to TARGET as source files. The name `speccpu2017_run_specpp` itself doesn't indicate that.


Repository:
  rT test-suite

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D101597/new/

https://reviews.llvm.org/D101597