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

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

jdenny added inline comments.


================
Comment at: llvm/utils/lit/lit/TestRunner.py:1277
+    """
+    A lit directive taking a shell command line.  For example,
+    'RUN: echo hello world'.
----------------
jhenderson wrote:
> 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).
I'm not sure if it matters to your point, but MY_RUN doesn't override RUN. It's an additional keyword parsed as a COMMAND.

In any case, I feel more comfortable using the term Command because that's the precedent already set by _handleCommand and ParserKind.COMMAND.  If we want to rethink that terminology, I feel like that should be a separate patch.


================
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
----------------
jdenny wrote:
> jhenderson wrote:
> > There's nothing particularly wrong with this, but why the two run lines in this case (unlike the define case)?
> I've forgotten.  I think it was a lazy way to make sure this check is performed when continuation lines are themselves continued.
> 
> For now, I've made them consistent.  I'll think about the right way to check that case separately.
> 
I decided it's not worthwhile to roughly double the number of `unterminated` tests in order to check every combination of conditions.  Instead, I added only two to offer some assurance that unterminated continuation lines are checked for the new directives added by this patch:

* unterminated-define-continuation.txt
* unterminated-redefine-continuation.txt


================
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
----------------
jhenderson wrote:
> 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.
Done.


================
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.
----------------
jhenderson wrote:
> 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.
I've added a test to this file.


================
Comment at: llvm/utils/lit/tests/Inputs/shtest-define/ws-and-continuations.txt:8
+
+# White space is not required around the name or value.
+#
----------------
jhenderson wrote:
> Nit: whitespace is usually all one word, right?
I've seen it written both ways, but I should at least be consistent within the same code.  Thanks.


================
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
----------------
jhenderson wrote:
> In this comment you're being inconsistent :)
Oops.  :-)


================
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.
----------------
jhenderson wrote:
> What's with the "..."?
I've reworded.


================
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} =                                                        \
----------------
jhenderson wrote:
> 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.
Done.


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

https://reviews.llvm.org/D132513