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

[Dan Liew via Phabricator via llvm-commits]

delcypher added a comment.

Have you considered the different approach of having conditional `RUN` lines instead? E.g.

  RUN: echo this always runs
  RUN(feature): echo this only runs when feature is true
  RUN(!feature): echo this only runs when feature is false
  
  RUN(feature || feature2): echo this only runs where feature or feature2 is true.
  RUN(!(feature || feature2)): echo this only runs where feature and feature2 are both false

I think this would make `RUN` lines easier to read. It doesn't support the "else" case quite as cleanly (you have to duplicate the condition and then negate it on separate line) but I think that's pretty minor because the condition can easily be made a "feature" that is computed from other features (in a test suite's `lit.cfg.py`)



================
Comment at: llvm/docs/TestingGuide.rst:615
 
+``%if feature {<if branch>} %else {<else branch>}``
+
----------------
asavonic wrote:
> tra wrote:
> > Would any of the following be expected to work?
> > - `%if feature {do_something} | FileCheck %s`   -- this will probably fail as empty substitution would produce no output to check
> > - `%if feature {do_something | FileCheck %s}` -- this would probably work.
> > - `%if featureA { %if featureB {do_AB} %else {do_A_notB}} %else {do_notA}` -- I suspect we'll fail to parse it correctly.
> > 
> > I'd document that we currently can't nest those `%if/%else` and, maybe add the `%if feature {do_something | FileCheck %s}` as a canonical example of how to use it for conditional output-checking tests.
> > 
> > Would any of the following be expected to work?
> > - `%if feature {do_something} | FileCheck %s`   -- this will probably fail as empty substitution would produce no output to check
> 
> Right. If the feature is not available then we'll get ` | FileCheck %s`.
>  
> > - `%if feature {do_something | FileCheck %s}` -- this would probably work.
> 
> This does not work for verbose output: LIT formats a command into `echo "RUN at line #"; ${command};` and if `${command}` is an empty string we'll get `echo "RUN at line #"; ;`. Bash cannot parse the double semicolon at the end.
> 
> We can probably handle this as a special case.
> 
> > - `%if featureA { %if featureB {do_AB} %else {do_A_notB}} %else {do_notA}` -- I suspect we'll fail to parse it correctly.
> 
> Recursive substitution actually works if `config.recursiveExpansionLimit` is set in lit.cfg. I'll this case to the test.
> 
> > 
> > I'd document that we currently can't nest those `%if/%else` and, maybe add the `%if feature {do_something | FileCheck %s}` as a canonical example of how to use it for conditional output-checking tests.
> > 
> 
> As I mentioned above, a canonical example would be:
> `%if feature {do_something | FileCheck %s} %else {true}` 
> 
I am not a fan of this syntax.

1. Being prefixed with `%` suggests it is substitution, but `%if` and `%else` are not normal substitutions. It might be worth introducing different syntax for these.
2. Lit has this annoying problem with substitutions are prefixes of other substitutions (e.g. `%s` and `%source`) where the order that substitutions added matter. There's a possibility that by introducing `%if` and `%else` that this will break existing substitutions (e.g. `%if_something` and `%else_something`). You could avoid this by using something like `%if%` and `%else%`.


================
Comment at: llvm/utils/lit/tests/Inputs/shtest-if-else/test.txt:9
+
+# RUN: %if feature  \
+# RUN:   { echo     \
----------------
Does parsing more complicated expressions work? I would expect it to, so we should probably add tests for it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122569