[PATCH] D150856: [lit] Add %{for-each-file} substitution

[Aaron Ballman via Phabricator via llvm-commits]

aaron.ballman added a comment.

In D150856#4431699 <https://reviews.llvm.org/D150856#4431699>, @jdenny wrote:

> In D150856#4428905 <https://reviews.llvm.org/D150856#4428905>, @Endill wrote:
>
>>> https://gist.github.com/jdenny-ornl/82aae9c3706c88fedebc8c44ac2a6694
>>
>> So we're looking at 20% more noise in a large test suite. I'm not happy with that either.
>
> When comparing the character counts for the solution using `%{for-each-file}` and the solution above using DEFINE/REDEFINE, I see a 9% increase for the latter.
>
> But what does that number really mean anyway?

This matters for DR tests because we put ~100 tests in every file. There are several thousand DR tests in C++ and an additional 500 or so in C, so this adds a fair amount of noise to these test files.

> It boils down to the following.  When formatting the DR tests as lit tests, the `%{for-each-file}` solution requires adding the following header to each DR test:
>
>   //--- dr500: dup 372
>
> where the DEFINE/REDEFINE solution requires adding the following header instead:
>
>   //--- dr500: dup 372
>   // REDEFINE: %{file} = dr500.cpp
>   // RUN: %{run}

The DR tests already have the comment for generation of the cxx status page, now we're talking about adding two additional lines of markup for each test and those two lines add zero information the user cares about. What's more, we have to repeat information that can get out of sync (if the `//--- dr500:` part doesn't match the `%{file} = dr500.cpp` part, the test doesn't run properly) and we're requiring people to know there's this hidden connection between the comment and the `REDEFINE` line. And if you forget to add the `REDEFINE` line, the test will still run and pass, but can have potentially unexpected results (as far as our DR testing is concerned) due to Clang's diagnostic display heuristics, which is the entire reason we're discussing this `for-each-file` feature in the first place. Using `for-each-file` has its own issues (it's new and novel), but once the RUN lines are written, the only thing the test developer needs to write is the special comment they already had to write anyway for the DR status page to be generated.

> As far as I can tell, neither version introduces any significant maintenance burden to the DR test.  The former is just more concise..  But does that difference justify maintaining `%{for-each-file}` in lit?

IMO, yes. I think I'm now convinced that `DEFINE`/`REDEFINE`, while powerful and potentially plausible as a feature to solve this problem, is not really the right approach for DR testing. While I'm usually hesitant to add new lit functionality (I think those additions are powerful but make it harder for newcomers to the project), DR testing is a pretty specialized use case that has different needs than other testing. I think it's reasonable for both features to coexist; they both have plenty of utility but different tradeoffs.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150856