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

[Andrzej Warzynski via Phabricator via llvm-commits]

awarzynski added a comment.

Thanks for addressing my comments! Here's a few more from me.

I've only skimmed through your changes in "TestRunner.py".  I will try to take another look later. Unless @jhenderson approves in the meantime :)



================
Comment at: llvm/docs/TestingGuide.rst:624-627
+  The number of the line where this substitution is used, with an
+  optional integer offset.  These expand only if they appear
+  immediately in ``RUN:``, ``DEFINE:``, and ``REDEFINE:`` directives.
+  Occurrences in substitutions defined elsewhere are never expanded.
----------------
jdenny wrote:
> awarzynski wrote:
> > jdenny wrote:
> > > awarzynski wrote:
> > > > Why delete "This can be used in tests with multiple RUN lines, which reference test file's line numbers"?
> > > It seems misleading to me: these substitutions can be used even if there's only one RUN line.  Beyond that issue, I'm not sure what the sentence says that the first sentence doesn't already say more clearly.
> > > 
> > > If I've misunderstood the sentence and lost an important point, please let me know.  However, if we keep it, it might need to be generalized to make sense for the new directives.
> > > these substitutions can be used even if there's only one RUN line.
> > 
> > That's true, but do they make sense if there's only one `RUN` line? I don't think so 🤔 . I couldn't find a single example in tree though :( I wrote one myself:
> > ```
> > one two three
> > 
> > 
> > ; RUN: head -%(line+8) %s | tail +%(line+8) | FileCheck %s --check-prefix=BOTTOM
> > ; RUN: head -%(line+7) %s | tail +%(line+7) | FileCheck %s --check-prefix=BOTTOM
> > ; RUN: head -%(line-5) %s | tail +%(line-5) | FileCheck %s --check-prefix=TOP
> > 
> > ; TOP: one two three
> > ; BOTTOM: five six seven
> > 
> > 
> > five six seven
> > ```
> >  To me that sentence helps understand the rationale for having `%(line+<number>)`/`%(line-<number>)` (as opposed to plain `%(line)`). Without it I'm just not sure what those substitutions are for.
> I have not found examples of line number substitutions either.  I even grepped through the LLVM history.
> 
> To me, saying line number substitutions don't make sense unless there are multiple RUN lines suggests that the RUN lines would refer to each other's line numbers.  When is that useful?
> 
> I can imagine a RUN line referring to another part of the file, such as source code that is expected to produce a diagnostic including a line number.  Only one RUN line is required for that to make sense.  For example:
> 
> ```
> <text>foo</txt>
> # RUN: tool-under-test %s 2>&1 | diag-check-tool --line-number=%(line-1) 'error: unmatched <text>'
> ```
> 
> But we normally use FileCheck or clang's -verify to verify output, and they have their own line number support, so the above use case probably doesn't come up in practice.
> 
> Does the example from your comment above make sense with a single RUN line?  I think so.  In that example, the line number substitutions are used to refer to other parts of the test file, but not specifically to the other RUN lines.  One RUN line is enough for them to be useful.  Did I misunderstand the intent?
> 
> If you feel the sentence in question describes a real use case, I might just add the phrase "For example," before it so at least it doesn't imply that's the only use case.
> 
> (By the way, I do show an example of using non-relative line number substitutions in D133172.)
> If you feel the sentence in question describes a real use case, I might just add the phrase "For example," before it so at least it doesn't imply that's the only use case.

I don't feel too strongly about this. To me it sounds like we are both unsure about the rationale for having `%(line)` there in the first place. For this reason, I would suggest either leaving that sentence intact or removing support for `%(line)` altogether (after having this proposed and accepted on Discourse). However, ....

> (By the way, I do show an example of using non-relative line number substitutions in D133172.)

... there is going to be an in-tree example soon :)

I'm happy with whatever you decide here. Personally I'd consider removing `%(line)`, but that's beyond the scope of this patch!


================
Comment at: llvm/docs/TestingGuide.rst:694-696
+    ; DEFINE: %{triple} =
+    ; DEFINE: %{cflags} =
+    ; DEFINE: %{fc-prefix} =
----------------
jdenny wrote:
> awarzynski wrote:
> > jdenny wrote:
> > > awarzynski wrote:
> > > > What happens if these remain undefined? I couldn't find this documented or tested.
> > > If, for example, `%{triple}` is left undefined, then the `REDEFINE:` directive for `%{triple}` below will report an error.  This error is documented under the `REDEFINE:` documentation entry below, and `llvm/utils/lit/tests/Inputs/shtest-define/errors/defined-check/redefine-none.txt` tests that behavior.  Is that what you're looking for?
> > >  Is that what you're looking for?
> > 
> > No :) By "undefined" I meant "empty". My bad, sorry! Basically, I had this scenario in mind:
> > ```
> >  ; DEFINE: %{triple} =
> >  ; DEFINE: %{cflags} =
> >  ; DEFINE: %{fc-prefix} =
> > 
> >   ; DEFINE: %{check} =                                                  \
> >   ; DEFINE:   %clang_cc1 -verify -fopenmp -fopenmp-version=51 %{cflags} \
> >   ; DEFINE:              -triple %{triple} -emit-llvm -o - %s |         \
> >   ; DEFINE:     FileCheck -check-prefix=%{fc-prefix} %s
> > ```
> > 
> > What should happen here?
> > 
> > > If, for example, %{triple} is left undefined, then the REDEFINE: directive for %{triple} below will report an error. 
> > 
> > So this is allowed:
> > ```
> > ; DEFINE: %{triple} =
> > ; REDEFINE: %{triple} =  
> > 
> > ```
> > and this is not allowed (assuming that there's no `DEFINE: %{triple}` in any of the local config files):
> > ```
> > ;  REDEFINE: %{triple} =
> > ```
> > Makes sense!
> > ```
> >  ; DEFINE: %{triple} =
> >  ; DEFINE: %{cflags} =
> >  ; DEFINE: %{fc-prefix} =
> > 
> >   ; DEFINE: %{check} =                                                  \
> >   ; DEFINE:   %clang_cc1 -verify -fopenmp -fopenmp-version=51 %{cflags} \
> >   ; DEFINE:              -triple %{triple} -emit-llvm -o - %s |         \
> >   ; DEFINE:     FileCheck -check-prefix=%{fc-prefix} %s
> > ```
> > 
> > What should happen here?
> 
> I think I get the point now: the initial values for `%{triple}` and `%{fc-prefix}` are unusable, so they must be redefined before `%{check}` is used in a RUN line, or the RUN line will malfunction I see how that quirk is confusing for an introductory example, so I've rewritten the example so that empty values work fine. I've also made it so every use of `%{check}` redefines every parameter substitution rather than just some, and I think that makes the example easier to read too. Let me know if it's still confusing.
> 
> > > If, for example, %{triple} is left undefined, then the REDEFINE: directive for %{triple} below will report an error. 
> > 
> > So this is allowed:
> > ```
> > ; DEFINE: %{triple} =
> > ; REDEFINE: %{triple} =  
> > 
> > ```
> > and this is not allowed (assuming that there's no `DEFINE: %{triple}` in any of the local config files):
> > ```
> > ;  REDEFINE: %{triple} =
> > ```
> > Makes sense!
> 
> Right.
> I've rewritten the example so that empty values work fine.

Nice, this is much clearer now. Thank you!


================
Comment at: llvm/utils/lit/lit/TestRunner.py:1221
 
+class ScriptDirective(object):
+    def __init__(self, start_line_number, end_line_number, keyword):
----------------
Docstring? Same for other classes/method.


================
Comment at: llvm/utils/lit/lit/TestRunner.py:1247
+
+    def add_continuation(self, line_number, keyword, line):
+        if keyword != self.keyword or not self.needs_continuation():
----------------
`ScriptDirective.add_continuation` takes only 3 arguments.


================
Comment at: llvm/utils/lit/lit/TestRunner.py:1315
+                "characters, hyphens, underscores, and colons"
+                .format(name=self.name))
+
----------------
[nit] You could use f-strings instead.


================
Comment at: llvm/utils/lit/tests/Inputs/shtest-define/lit.cfg:27-28
+
+# These cannot be redefined by 'REDEFINE:', which doesn't know which one to
+# redefine.
+config.substitutions.insert(0, ('%{global:multiple-exact}', 'first'))
----------------
jdenny wrote:
> awarzynski wrote:
> > And I guess that such usage should be discouraged?
> I would say so.   If you use `%{global:multiple-exact}` somewhere, which value do you expect it to have?
My point is that this particular example could suggest that this usage is //fine//. I would add a comment explaining that this is bad practice and is only used here to facilitate testing. 


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

https://reviews.llvm.org/D132513