[PATCH] D65121: [FileCheck][lit] Make FILECHECK_OPTS useful for their test suites

[Paul Robinson via Phabricator via llvm-commits]

probinson added a comment.

Regarding the FileCheck part, it should be split off to its own patch and the mass modification of FileCheck's test suite incorporated, because it's hard to grasp the effects without seeing those.

I have to say my initial reaction was "omg not another layer of indirection in the FileCheck test suite" but rereading PR40284, my own comments there have persuaded me to rethink that position. :-)

Reading the comments in FileCheck/lit.local.cfg, yes at some low level the point is whether varying the output affects the test, but I don't think that's a properly principled way to look at it.  Also thinking that way is likely to cause brain damage in anyone coming along later and trying to decipher when to use which spelling of "run this tool."

Better to look at it this way (this is a multi-paragraph argument, sorry):  This is a tool, these are its tests, and we want to run the tool and then FileCheck its output.  This is how we test llvm-objdump and llvm-dwarfdump and all the rest of those tools.  By a bizarre quirk of fate, the command to run the tool itself is spelled the same as FileCheck.  So, let's change the spelling of "run this tool" because we'd like to keep the RUN lines following a pattern of `toolname | FileCheck`.  We can call it something like %FileCheckUnderTest or %TestFileCheck or something else that implies "run the FileCheck I'm trying to test." (This would happen to be implemented as "run FileCheck with no environment.") Then your basic test of FileCheck would have a RUN line something like `RUN: %TestFileCheck blah | FileCheck %s` and it should be clear to the easily-trained eye which invocation of FileCheck is fulfilling which role.  Importantly the *choice* of which spelling to use is based solely on the role within the test RUN line, not some more complicated "does it affect the test result" decision.

Sadly there are a few cases where a test actually does want to set an environment variable, and in those cases the above substitution will not do the right thing. Apparently we want (or also want) a substitution that will clean out the environment but not actually run the tool, so that we can set new environment variables before running it.  Maybe call it %CleanEnv.  Then your basic FileCheck-with-environment test's RUN line would look something like `%CleanEnv env FILECHECK_OPTS=-v FileCheck blah | FileCheck %s`

At that point, `%TestFileCheck` can be considered shorthand for `%CleanEnv FileCheck` (although likely the substitution wouldn't actually be implemented that way).

WDYT?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65121