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

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/docs/TestingGuide.rst:615
 
+``%if feature {<if branch>} %else {<else branch>}``
+
----------------
tra wrote:
> delcypher wrote:
> > 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%`.
> > 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.
> 
> Perhaps we should expand the empty branch to `:` which *is* the standard null command in shell https://pubs.opengroup.org/onlinepubs/009695399/utilities/colon.html
> 
> Perhaps we should expand the empty branch to : which *is* the standard null command in shell https://pubs.opengroup.org/onlinepubs/009695399/utilities/colon.html

Don't forget Windows (where colons are not null actions).

> 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%`.

I'm not convinced about adding more substitutions, especially `%if%` style since that's the canonical way of specifying environment variables in a Windows command prompt. %if/%else is pretty intuitive, I reckon. I personally don't think 2 is a real issue, unless there is already existing problems within the LLVM code-base like this. Yes, there might be downstream users, but a) we don't bend over backwards to avoid breaking them, and b) there's a clear path forward (rename the downstream substitution).


================
Comment at: llvm/docs/TestingGuide.rst:615
 
+``%if feature {<if branch>} %else {<else branch>}``
+
----------------
jhenderson wrote:
> tra wrote:
> > delcypher wrote:
> > > 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%`.
> > > 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.
> > 
> > Perhaps we should expand the empty branch to `:` which *is* the standard null command in shell https://pubs.opengroup.org/onlinepubs/009695399/utilities/colon.html
> > 
> > Perhaps we should expand the empty branch to : which *is* the standard null command in shell https://pubs.opengroup.org/onlinepubs/009695399/utilities/colon.html
> 
> Don't forget Windows (where colons are not null actions).
> 
> > 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%`.
> 
> I'm not convinced about adding more substitutions, especially `%if%` style since that's the canonical way of specifying environment variables in a Windows command prompt. %if/%else is pretty intuitive, I reckon. I personally don't think 2 is a real issue, unless there is already existing problems within the LLVM code-base like this. Yes, there might be downstream users, but a) we don't bend over backwards to avoid breaking them, and b) there's a clear path forward (rename the downstream substitution).
> 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/shtest-if-else.py:19
+# CHECK-NEXT: Exit Code: 0
+
----------------
Nit: too many blank lines at EOF.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122569