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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

Mostly just comment nitpicks from me. I don't really have time to think about test coverage, so that's about it from me, aside from any follow-up comments.



================
Comment at: llvm/utils/lit/lit/TestRunner.py:1277
+    """
+    A lit directive taking a shell command line.  For example,
+    'RUN: echo hello world'.
----------------
jdenny wrote:
> jhenderson wrote:
> > 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.
> > Are there any cases where this isn't a `RUN` directive?
> 
> Yes.  See `MY_RUN` in `llvm/utils/lit/tests/Inputs/testrunner-custom-parsers/test.txt` and `llvm/utils/lit/tests/unit/TestRunner.py`.
> 
> I chose "CommandDirective" for consistency with `_handleCommand` and `ParserKind.COMMAND`.
> 
> > Similarly, are there any cases where a `RUN` directive isn't this?
> 
> I don't think so.
> 
To clarify, I meant, any cases other than if someone specifies a different name for the "RUN" directive explicitly. Given that that is a specific overriding of the default behaviour, I think my suggestion still stands, but up to you (I don't feel strongly about it).


================
Comment at: llvm/utils/lit/tests/Inputs/shtest-define/expansion-order.txt:1
+# Redefine the parameter of %{global:echo} before using it: %{global:greeting}.
+# The necessary expansion order was established in the test suite config
----------------
Not sure I follow the bit after the colon? Are you saying that `%{global:greeting}` is the parameter for `%{global:echo}`? If so, I'd just move the name to between "the" and "parameter" earlier in the line.


================
Comment at: llvm/utils/lit/tests/Inputs/shtest-define/ws-and-continuations.txt:1
+# Empty values are permitted and reasonable, especially when just establishing
+# expansion order.
----------------
Just wondering if it's worth a test case with two adjacent substitutions without separating whitespace? I.e. something like `%{foo}%{bar}`? I'd expect both to expand, with no separating whitespace added. Not sure where the test case would belong.


================
Comment at: llvm/utils/lit/tests/Inputs/shtest-define/ws-and-continuations.txt:8
+
+# White space is not required around the name or value.
+#
----------------
Nit: whitespace is usually all one word, right?


================
Comment at: llvm/utils/lit/tests/Inputs/shtest-define/ws-and-continuations.txt:18
+
+# Exact white space is preserved within the value, but whitespace enclosing the
+# name or value is discarded.  ('%{' and '}' are part of the name, and
----------------
In this comment you're being inconsistent :)


================
Comment at: llvm/utils/lit/tests/Inputs/shtest-define/ws-and-continuations.txt:68
+
+# A line continuation is permitted anywhere... that whitespace is permitted
+# because it reduces to a single space.
----------------
What's with the "..."?


================
Comment at: llvm/utils/lit/tests/shtest-define.py:11-12
+# DEFINE: %{fc-args} =
+# DEFINE: %{record-test} =                                                     \
+# DEFINE:   echo '  shtest-define :: %{test}' >> %t.tests.actual.txt
+# DEFINE: %{run-test} =                                                        \
----------------
Nit: a slightly more natural flow would be to have the `record-test` substitution after the `run-test` one, since that is the order they're used in.


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

https://reviews.llvm.org/D132513