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

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

jdenny added a comment.

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

> I've done a bit of a review. Not looked at the testing yet, and I can't say to have fully grasped the details of everything that's going on in the python at this point.

Thanks for the review so far.



================
Comment at: llvm/docs/TestingGuide.rst:746
+  the insertion behavior of the ``DEFINE:`` and ``REDEFINE:`` directives is
+  specified below and is designed specifically for the use case presented in the
+  example above.
----------------
jhenderson wrote:
> Nit: I believe it should be "use-case" (hyphenated).
Interesting.  I didn't recall it was ever written that way.  <https://www.merriam-webster.com/dictionary/use%20case> claims that's less common.  <https://en.wikipedia.org/wiki/Use_case> spells it without the hyphen.  A couple greps in llvm-project.git:

```
$ git grep -i 'use-case' | wc
     47     509    5139
$ git grep -i 'use\s*case' | wc
    276    3310   30237
```

The hyphen makes sense to me if the phrase is being used as an adjective, but here's it a noun.


================
Comment at: llvm/utils/lit/lit/TestRunner.py:1280
+        # enclosing each.
+        parts = self.body.split('=', 1)
+        if len(parts) == 1:
----------------
jhenderson wrote:
> Is there a test to show that you can have substitutions containing `=` characters in the value? (I skimmed briefly, but there's a lot of tests, so I may not have spotted it).
Good point.  Added one.


================
Comment at: llvm/utils/lit/lit/TestRunner.py:1301
+        # separated by a comma.  Requiring a leading letter avoids that.
+        if not re.fullmatch(r'%{[a-zA-Z][-_:0-9a-zA-Z]*}', self.name):
+            raise ValueError(
----------------
jhenderson wrote:
> Not sure if it's worth caring about, but normally `_` would be a valid leading character in an identifier in C and Python, so not permitting it here might cause a little confusion.
For the leading character, I was following the FileCheck prefix documentation.  It turns out that doesn't match the FileCheck prefix implementation.

OK, I've extended it to accept the leading `_`, and I've added a test that all the above characters are indeed accepted.


================
Comment at: llvm/utils/lit/lit/TestRunner.py:1304
+                "Substitution name '{name}' is malformed as it must start with "
+                "'%{{', it must end with '}}', and the rest must must start "
+                "with a letter and contain only alphanumeric characters, "
----------------
jhenderson wrote:
> "must must" -> "must" (and presumably fix a corresponding test).
Good catch.  Fixed.

The associated tests check the diagnostic only up to the word "malformed", so there's nothing to update.  That was my thinking when I wrote them: don't maintain the verbose description of the syntax repeatedly in the test suite.  I'm generally not very consistent about how much of a diagnostic to replicate in a test suite, so let me know if you think I should include a different amount in this case.


================
Comment at: llvm/utils/lit/lit/TestRunner.py:1497
+    output = []
+    for dir in script:
+        if isinstance(dir, SubstDirective):
----------------
jhenderson wrote:
> `dir` is a built-in python command, so I'd pick a different name. I also find it a confusing name anyway, because I'd expand it to "directory" not "directive". Any reason not to just name it "directive"?
Good point.  Done.


================
Comment at: llvm/utils/lit/lit/TestRunner.py:1270-1272
+        # '\' is documented as indicating a line continuation even if whitespace
+        # separates it from the newline.  It looks like a line continuation, and
+        # it would be confusing if it didn't behave as one.
----------------
jhenderson wrote:
> How come this only applies to substitutions and not commands?
`_handleCommand` already does the rstrip, so the effect is the same.  Would it help if I moved that logic into `CommandDirective`?


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

https://reviews.llvm.org/D132513