[PATCH] D122569: [lit] Support %if ... %else syntax for RUN lines

[Joel E. Denny via Phabricator via llvm-commits]

jdenny requested changes to this revision.
jdenny added a comment.
This revision now requires changes to proceed.

@asavonic: Thanks for working on this.  I've pointed out a few issues that need to be discussed before this lands.  I also need to make another pass before I'm ready.



================
Comment at: llvm/utils/lit/lit/TestRunner.py:54
+# empty as a result of conditinal substitution.
+kPdbgRegex = '%dbg\\(([^)\'"]*)\\)(.*)'
 
----------------
It appears this change and related changes below make it possible to have blank `RUN:` lines in general, even when not using `%if`.  That's probably worth mentioning in the review summary.

Also, the summary misspells `%else` as `else` a few times.  Please fix.


================
Comment at: llvm/utils/lit/lit/TestRunner.py:1220
         # Apply substitutions
         for a,b in substitutions:
             if kIsWindows:
----------------
This processes `%if` and `%else` after other substitutions.  That means a test suite's config could (probably unwittingly) interfere with lit's normal `%if` and `%else` processing by defining its own `%if` and `%else` substitutions.

I think lit shouldn't permit that.  One way to prevent that is to process `%if` and `%else` before other substitutions.  You'd also need to move the `escape(ln)` call before that.


================
Comment at: llvm/utils/lit/lit/TestRunner.py:1232
 
+        if_regex = r'%if\s+(.+?)\s+{(.*?)}(?:\s+%else\s+{(.*?)})?'
+        if_match = _caching_re_compile(if_regex).search(ln)
----------------
Is requiring a space before each `{` and after `}` necessary?  If omitted, the whole if-else won't be unrecognized.  I think that would be confusing.

Eventually, it would be nice to parse this properly: match `%if`, then search for the condition and complain if it's not there, then search for the `{` and complain if it's not there, etc.


================
Comment at: llvm/utils/lit/tests/Inputs/shtest-if-else/test.txt:7
+# RUN: %if feature { echo "test-1" }
+# CHECK-NEXT: '{{RUN}}: at line 6'; echo "test-1"
+
----------------
Please use `[[#@LINE-1]]` instead of 6 (likewise below) so that we can remove and add checks without renumbering throughout the rest of the file.


================
Comment at: llvm/utils/lit/tests/Inputs/shtest-if-else/test.txt:51
+#
+# RUN: echo %if feature { %if feature {"test-9"} }
+# CHECK-NEXT: '{{RUN}}: at line 51'; echo "test-9"
----------------
This case happens to work, but nested expressions do not work in general.  It needs a proper parser with a stack to handle that.

The regex for the outer `%if` matches until the inner `}` because of the minimal munch `.*?` in the regex.  That leaves behind the outer `}` for the inner `%if`.  Lucky.

But what happens in the case of an outer `nofeature`?  The left-behind `}` results in `echo }`.  Also consider cases with inner and outer `%else`.

Non-nested `%if` cases involving braces don't work either:

```
%if have_sed { sed 's/<a\{2,3\}>/<FOUND>/g' %s | FileCheck %s }
```

By the way, it might be worthwhile to add `{{$}}` to the end of the FileCheck patterns in this file to catch any portion of the `RUN` unexpectedly left behind.  FileCheck's `--match-full-lines` plus `{{^.*}}` at the start of FileCheck patterns might be even better.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122569

STAMPS
actor(@jdenny) application(Differential) author(@asavonic) herald(H223) herald(H297) herald(H428) herald(H498) herald(H576) herald(H615) herald(H864) mention(@asavonic) monogram(D122569) object-type(DREV) phid(PHID-DREV-vuwwzxnlgkasuhumt4ze) reviewer(@jdenny) reviewer(@jhenderson) reviewer(@mstorsjo) reviewer(@probinson) reviewer(@tra) reviewer(@yln) revision-repository(rG) revision-status(needs-revision) subscriber(@delcypher) subscriber(@i-chebykin) subscriber(@llvm-commits) tag(#all) tag(#llvm) via(web)