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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

I've run out of time to look at this further for now, but hopefully should be able to go through the rest of the testing later in the week.



================
Comment at: llvm/docs/TestingGuide.rst:693-694
+For example, the following directives can be inserted into a test file to define
+a ``%{check}`` substitution as well as empty initial values for substitutions
+that serve as the parameters of ``%{check}``:
+
----------------
The part from "as well as" doesn't read well for me. I think it's probably clearer to rephrase as "... to define %{cflags} and %{fcflags} substitutions with empty initial values, which serve as the parameters of another newly-defined %{check} substitution:". WDYT?




================
Comment at: llvm/docs/TestingGuide.rst:776-777
+   pattern is ``%{name}``, or it reports an error if there are no substitutions
+   with that pattern or if there are multiple substitutions whose patterns
+   contain ``%{name}``.  The substitution's current position in the substitution
+   list does not change so that expansion order relative to other existing
----------------
On first read, I thought the reference to patterns containing `%{name}` meant something like `%{foo}` couldn't be referenced with a definition/redefinition if `%{afooa}` existed. Is it actually meant to be referring to something like `%{a%{foo}a}`? If so, I feel like an example here could be useful. If not (i.e. `%{foo}` can't be used if e.g. `%{afooa}` is defined), I find this to be a problem, as I think it is not unreasonable to have e.g. ``%{check}` and `%{check2}` in the same substitution set.


================
Comment at: llvm/docs/TestingGuide.rst:796-797
+      will match part of ``%{name}`` or vice-versa, producing confusing
+      expansions.  However, the patterns of substitutions not defined by these
+      directives are not restricted to this form, so overlaps are still
+      theoretically possible.
----------------
Would it be clearer here to refer to the config files, since I believe that's the only way to specify an "illegal" pattern? Something like "However, substitution patterns defined in configuration files are not restricted to this form...".


================
Comment at: llvm/docs/TestingGuide.rst:822-823
+if substitutions are not defined in the proper order, some will remain in the
+``RUN:`` line unexpanded.  For example, against the advice of the previous
+section, the following directives refer to ``%{inner}`` within ``%{outer}`` but
+do not define ``%{inner}`` until after ``%{outer}``:
----------------
I'm not sure the bit about "against the advice" makes a huge amount of sense, since the previous section explained the situation and what happens, I thought? I'd just omit the bit between the commas, personally.


================
Comment at: llvm/docs/TestingGuide.rst:832-833
+
+Because ``DEFINE:`` inserts substitutions at the start of the substitution list,
+``%{inner}`` expands first but has no effect because the original ``RUN:`` line
+doesn't contain ``%{inner}``.  Next, ``%{outer}`` expands, and the output of the
----------------
I'd suggest replacing the first "Because" with something else, e.g. "Since", to avoid the double because in this sentence.


================
Comment at: llvm/docs/TestingGuide.rst:857
+
+To improve performance, lit will stop make passes when it notices the ``RUN:``
+line has stopped changing.  In the above example, setting the limit higher than
----------------



================
Comment at: llvm/utils/lit/lit/TestRunner.py:1277
+    """
+    A lit directive taking a shell command line.  For example,
+    'RUN: echo hello world'.
----------------
Are there any cases where this isn't a `RUN` directive? Similarly, are there any cases where a `RUN` directive isn't this? If not to both, I'd consider renaming to `RunDirective` so that it marries up with the actual name used in the test files.


================
Comment at: llvm/utils/lit/lit/TestRunner.py:1302
+    """
+    A lit directive taking a substitution definition.  For example,
+    'DEFINE: %{name} = value'.
----------------
I'd suggest this slight addition.


================
Comment at: llvm/utils/lit/lit/TestRunner.py:1329
+            raise ValueError("Substitution's continuation is empty")
+        # Append line.  Replace the '\' and adjacent space with a single space.
+        self.body = self.body.rstrip()[:-1].rstrip() + ' ' + line.lstrip()
----------------
I think this is strictly more correct, which could be important in some edge cases, I guess.


================
Comment at: llvm/utils/lit/tests/Inputs/shtest-define/errors/continuation/unterminated-run-redefine.txt:3-4
+
+# RUN: echo \
+# RUN:   Hello \
+# REDEFINE: %{global:what}=bar
----------------
There's nothing particularly wrong with this, but why the two run lines in this case (unlike the define case)?


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

https://reviews.llvm.org/D132513