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

[Andrzej Warzynski via Phabricator via llvm-commits]

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


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


================
Comment at: llvm/utils/lit/tests/Inputs/shtest-define/lit.cfg:14
+# of %{global:what}, so we make sure the former expands before the latter.
+# If we always insert at the beginning fo the substitution list (as DEFINE
+# does), then the rule is simple: define a substitution before you refer to it.
----------------



================
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}>', '<@>'))
----------------
Why not?


================
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'))
----------------
And I guess that such usage should be discouraged?


================
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}', '@'))
----------------
Why not? And what's the difference between `%{global:multiple-once-exact}` and `<%{global:multiple-once-exact}>`?


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

https://reviews.llvm.org/D132513