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

[Andrew Savonichev via Phabricator via llvm-commits]

asavonic added inline comments.


================
Comment at: llvm/utils/lit/lit/TestRunner.py:54
+# empty as a result of conditinal substitution.
+kPdbgRegex = '%dbg\\(([^)\'"]*)\\)(.*)'
 
----------------
jdenny wrote:
> 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.
Thanks! Fixed the review summary.


================
Comment at: llvm/utils/lit/lit/TestRunner.py:1220
         # Apply substitutions
         for a,b in substitutions:
             if kIsWindows:
----------------
jdenny wrote:
> 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.
Thank you, that makes sense. Moved `%if..%else` processing before other substitutions.


================
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)
----------------
jdenny wrote:
> 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.
Right, spaces are no longer required in the last revision of the patch.


================
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"
+
----------------
jdenny wrote:
> 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.
Done.


================
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"
----------------
jdenny wrote:
> 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.
Right, I was hoping that we can live with a regex parser for now, but with all these issues it is unacceptable. I've re-implemented the parser and now it should support all these cases well enough. We also no longer need `recursiveExpansionLimit` flag to be set for nested `%if..%else` expressions.


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

https://reviews.llvm.org/D122569