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

Mon Sep 12 09:05:21 PDT 2022

jdenny marked an inline comment as done and an inline comment as not done.
jdenny added a comment.

@awarzynski, @jhenderson: Thanks!

================
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
----------------
jhenderson wrote:
> 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.
> 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}`?

The full `%{name}` is the substitution's pattern, so the latter is what I meant.

>  If so, I feel like an example here could be useful.

Done.

================
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.
----------------
jhenderson wrote:
> 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...".
The other possibility I was thinking of is a substitution always defined by lit itself.  For example, it's best not to define a substitution whose pattern starts with `%s`.

That's relevant to the first part of the bullet (why braces are required here).  However, as far as I know, it's not currently relevant to the second part (I know of no existing lit-defined substitution whose pattern would overlap with the braced form).  Lit evolves constantly, so that could change.  For example, what if someone thinks `%{name([0-9]*)}` is a reasonable pattern (where parens are capturing)?

I've expanded "not defined by these directives" to mention both sources of substitutions.  Let me know if that's not clear enough.

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

I was trying to make sure the reader knows this example is not the recommended way to write directives.  Instead, I've added a comment next to the directives, which should help anyone skimming the documentation for examples and not reading the text.

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

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

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

https://reviews.llvm.org/D132513