[PATCH] D132513: [lit] Implement DEFINE and REDEFINE directives
James Henderson via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Sep 12 02:53:16 PDT 2022
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
More information about the llvm-commits
mailing list