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

Mon Sep 5 16:11:45 PDT 2022

jdenny added inline comments.

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

================
Comment at: llvm/docs/TestingGuide.rst:694-696
+    ; DEFINE: %{triple} =
+    ; DEFINE: %{cflags} =
+    ; DEFINE: %{fc-prefix} =
----------------
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.

================
Comment at: llvm/utils/lit/tests/Inputs/shtest-define/lit.cfg:21-22
+
+# Neither 'DEFINE:' or 'REDEFINE:' can (re)define any of the '%{global:*}'
+# contained in these patterns.
+config.substitutions.insert(0, ('<%{global:inside}>', '<@>'))
----------------
awarzynski wrote:
> Why not?
I extended the comments here to explain.  TestingGuide.rst documents the restriction generally.

================
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'))
----------------
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?

================
Comment at: llvm/utils/lit/tests/Inputs/shtest-define/lit.cfg:32-33
+
+# '%{global:multiple-once-exact}' cannot be redefined by 'REDEFINE:' even though
+# it's only defined once.
+config.substitutions.insert(0, ('%{global:multiple-once-exact}', '@'))
----------------
awarzynski wrote:
> Why not? And what's the difference between `%{global:multiple-once-exact}` and `<%{global:multiple-once-exact}>`?
> Why not?

I extended the comments here to explain.  TestingGuide.rst documents the restriction generally.

> And what's the difference between `%{global:multiple-once-exact}` and `<%{global:multiple-once-exact}>`?

I'm not sure I understand the question.  One pattern simply contains the other.  Can you rephrase?

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

https://reviews.llvm.org/D132513