[PATCH] D132513: [lit] Implement DEFINE and REDEFINE directives

[Joel E. Denny via Phabricator via llvm-commits]

jdenny added a comment.

In D132513#3744850 <https://reviews.llvm.org/D132513#3744850>, @jhenderson wrote:

> I haven't got the time to review this right now, but I wanted to say a big thank you for working on this.

Sure.  Thanks for the quick response.

> I experimented with a CHECK-DEFINE pattern for FileCheck, since I only needed it in the context of FileCheck check patterns, but this will do the job perfectly:

Makes sense.

> One question I do have: why do we need both DEFINE and REDEFINE? I personally would think that just DEFINE would be enough, with it implicitly replacing any existing substitution. Or is it something to do with substitution order?

I agree that DEFINE would be enough for the core functionality.  However, I find that debugging complex substitution expansions can sometimes be painful.  I split DEFINE into two directives in order to improve readability and facilitate debugging, as follows.

I work with test suites where lit configuration files define substitutions in terms of other substitutions. Let's say a lit configuration file defines a substitution foo such that its value contains a substitution bar.  Now let's say you have a test file that uses foo in a RUN line, and you don't realize that foo uses bar or that bar is a thing at all.  However, bar seems like a nice name for a per-test substitution that you want, so you define bar using DEFINE.  If DEFINE doesn't complain that bar is already defined, you've corrupted the RUN line, and you'll be lucky if the RUN line fails so you have the opportunity to debug and eventually figure out your mistake.  On the other hand, if DEFINE does complain, you will immediately discover your mistake.

Another scenario is where you intend to redefine a substitution but mistype it.  Such things do happen: think of all the ways people have tried to diagnose typos in FileCheck prefixes.  Again, hopefully the next RUN line will fail and you'll eventually figure out your mistake... or maybe it won't fail given that the intended substitution's stale value didn't produce a failure in previous RUN lines.  On the other hand, if REDEFINE complains that the substitution is not already defined, you will immediately discover your mistake.

Finally, I find that substitution expansion order can be confusing at times.  This patch tries to address that confusion with a definition-before-reference rule.  However, if the only directive is DEFINE, then how do you tell where the original definitions are?  Sure, you could look for the first DEFINE for a substitution, but what if the first DEFINE in the file isn't the first definition because there's a definition in the lit configuration file?  I think it's easier to understand expansion order if DEFINE always sets substitution expansion order and REDEFINE never does.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132513