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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

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.



================
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.
----------------
Nit: I believe it should be "use-case" (hyphenated).


================
Comment at: llvm/utils/lit/lit/TestRunner.py:1259
+        self._parse_body()
+    def add_continuation(self, line_number, keyword, line):
+        if keyword != self.keyword or not self.needs_continuation():
----------------
Nit: I'd probably put blank lines between functions, as in some cases, it's easy to miss that you've entered a new function.


================
Comment at: llvm/utils/lit/lit/TestRunner.py:1280
+        # enclosing each.
+        parts = self.body.split('=', 1)
+        if len(parts) == 1:
----------------
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).


================
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(
----------------
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.


================
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, "
----------------
"must must" -> "must" (and presumably fix a corresponding test).


================
Comment at: llvm/utils/lit/lit/TestRunner.py:1497
+    output = []
+    for dir in script:
+        if isinstance(dir, SubstDirective):
----------------
`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"?


================
Comment at: llvm/utils/lit/lit/TestRunner.py:1504
+            else:
+                # can come from preamble_commands
+                assert isinstance(dir, str)
----------------
Nit


================
Comment at: llvm/utils/lit/lit/TestRunner.py:1524
     CUSTOM: A keyword with custom parsing semantics.
+    SUBST: A keyword taking a LIT substitution definition.  Ex
+        'DEFINE: %{name}=value'
----------------
Nit: double space after period is inconsistent with the rest of this comment.


================
Comment at: llvm/utils/lit/lit/TestRunner.py:1642
+        line = line.rstrip()
+        # Substitute line number expressions
+        line = cls._substituteLineNumbers(line_number, line)
----------------
Nit: whilst moving this, might as well add the missing trailing full stop.


================
Comment at: llvm/utils/lit/lit/TestRunner.py:1760-1761
     # Verify the script contains a run line.
-    if require_script and not script:
+    if require_script and not any(isinstance(dir, CommandDirective)
+                                  for dir in script):
         raise ValueError("Test has no 'RUN:' line")
----------------
Ditto comments re. `dir` name.


================
Comment at: llvm/utils/lit/lit/TestRunner.py:1770
+    # here.
+    for dir in script:
+        if dir.needs_continuation():
----------------
Ditto.


================
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.
----------------
How come this only applies to substitutions and not commands?


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

https://reviews.llvm.org/D132513