This is an archive of the discontinued LLVM Phabricator instance.

[lit] Support %if ... %else syntax for RUN lines
ClosedPublic

Authored by asavonic on Mar 28 2022, 4:08 AM.

Details

Summary

This syntax allows to modify RUN lines based on features
available. For example:

RUN: ... | FileCheck %s --check-prefix=%if windows %{W%} %else %{NON-W%}
CHECK-W: ...
CHECK-NON-W: ...

The whole command can be put under %if ... %else:

RUN: %if tool_available %{ %tool %} %else %{ true %}

or:

RUN: %if tool_available %{ %tool %}

If tool_available feature is missing, we'll have an empty command in this RUN line.
LIT used to emit an error for empty commands, but now it treats such commands as nop
in all cases.

Multi-line expressions are also supported:

RUN: %if tool_available %{ \
RUN:   %tool              \
RUN: %} %else %{             \
RUN:   true               \
RUN: %}

Background and motivation:
D121727 [NVPTX] Integrate ptxas to LIT tests
https://reviews.llvm.org/D121727

Diff Detail

Event Timeline

asavonic created this revision.Mar 28 2022, 4:08 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 28 2022, 4:08 AM
Herald added a subscriber: delcypher. · View Herald Transcript
asavonic requested review of this revision.Mar 28 2022, 4:08 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 28 2022, 4:08 AM
tra added inline comments.Mar 28 2022, 11:40 AM
llvm/docs/TestingGuide.rst
615

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.

llvm/utils/lit/tests/shtest-if-else.py
2–3

Why not just pipe the output straight to FileCheck and avoid the temp file.

Is

FileCheck %s --check-prefix={%if windows {W} else {NON-W}}

the main motivating example? You can already solve that by introducing a %check_prefix substitution in your lit.cfg.py file and expanding it to the correct thing depending on your platform. Would that suffice?

asavonic added inline comments.Mar 28 2022, 12:08 PM
llvm/docs/TestingGuide.rst
615

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}

asavonic edited the summary of this revision. (Show Details)Mar 28 2022, 12:13 PM

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)

llvm/docs/TestingGuide.rst
615

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%.
llvm/utils/lit/tests/Inputs/shtest-if-else/test.txt
10

Does parsing more complicated expressions work? I would expect it to, so we should probably add tests for it.

tra added inline comments.Mar 28 2022, 12:25 PM
llvm/docs/TestingGuide.rst
615

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

Is

FileCheck %s --check-prefix={%if windows {W} else {NON-W}}

the main motivating example? You can already solve that by introducing a %check_prefix substitution in your lit.cfg.py file and expanding it to the correct thing depending on your platform. Would that suffice?

The main motivation is integration of external tools (ptxas, see D121727), where (1) it is important to make it clear that a command may not execute if the corresponding tool is not available, and (2) we need to support output redirection, different options for different tests, etc. Doing this in lit.cfg is either not clear, or not flexible (see https://reviews.llvm.org/D121727?id=416283, where we have ptxas-verify implemented in lit.cfg in comparison with if-like syntax).
Having this feature implemented as a substitution allows us to use it for other things, like platform dependent prefixes.

jhenderson added inline comments.Mar 29 2022, 1:35 AM
llvm/docs/TestingGuide.rst
615

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

jhenderson added inline comments.Mar 29 2022, 1:35 AM
llvm/docs/TestingGuide.rst
615

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%.
llvm/utils/lit/tests/shtest-if-else.py
20

Nit: too many blank lines at EOF.

I see that for %if feature {A} %else {B} the space before the %else is not preserved in the result.
If there is no %else what happens? e.g., XX %if feature {YY} ZZ Asking because I see no test case for this.

I take it the %if expressions take only lit feature keywords, like REQUIRES? No triple substrings, as allowed by UNSUPPORTED/XFAIL?
(I have a back-burner task to make all three of those be consistent, this would be a fourth situation.)

I actually agree with @delcypher that RUN(feature): seems cleaner and certainly handles the motivating case very well. But it is obviously less powerful, and I won't object to the %if solution.

asavonic updated this revision to Diff 418953.Mar 29 2022, 12:34 PM
  • Added support for binary expressions.
  • Added handling for empty commands in RUN lines.
  • Added new tests.

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)

Both options should work fine for ptxas or any other external tools.
%if .. %else syntax is more generic and it is actually easier to implement in LIT: it is just a bit more complicated substitution, whereas RUN(cond) is completely new syntax that (may) require more work to enable (at least I couldn't figure out how to do this quickly).

I see that for %if feature {A} %else {B} the space before the %else is not preserved in the result.
If there is no %else what happens? e.g., XX %if feature {YY} ZZ Asking because I see no test case for this.

Spaces before %else are ignored only if %else is present. Added this test:

# RUN: echo XX %if feature {YY} ZZ
# RUN: echo AA %if feature {BB} %else {CC} DD
# RUN: echo AA %if nofeature {BB} %else {CC} DD

echo XX YY ZZ
echo AA BB DD
echo AA CC DD

I take it the %if expressions take only lit feature keywords, like REQUIRES? No triple substrings, as allowed by UNSUPPORTED/XFAIL?
(I have a back-burner task to make all three of those be consistent, this would be a fourth situation.)

Yes, identifiers are taken from config.available_features, just like for REQUIRES. The last patch adds binary expression evaluation, e.g. feature1 && !feature2 or regex match {{fea.+}}.

I actually agree with @delcypher that RUN(feature): seems cleaner and certainly handles the motivating case very well. But it is obviously less powerful, and I won't object to the %if solution.

Agree.

llvm/docs/TestingGuide.rst
615
  • %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.

Fixed this by changing how this debug print is implemented. Now we don't print an extra semicolon if the command is empty.

llvm/utils/lit/tests/Inputs/shtest-if-else/test.txt
10

Does parsing more complicated expressions work? I would expect it to, so we should probably add tests for it.

This is now implemented in the latest revision of the patch.

llvm/utils/lit/tests/shtest-if-else.py
2–3

Done.

20

Fixed.

tra added inline comments.Mar 29 2022, 1:04 PM
llvm/utils/lit/tests/shtest-if-else.py
2

I'd use the full name %{inputs}/shtest-if-else/test.txt.
This way it's obvious how the input file is used and it avoids accidentally picking up some other file we may eventually have in that directory.

Perhaps it would make sense to incorporate the test.txt into this file. E.g add another RUN line to generate the test input file and then run lit on it.
This would allow interleaving the RUN/check directives in the same source. Right now it's a bit of a pain to follow what we're exacuting and which output it's supposed to generate.

E.g. we could do something like this:

RUN: grep LITRUN %s | sed -e 's/LITRUN/RUN/' > %t/test.txt
RUN: grep LITCFG %s | sed -e 's/# LITCFG://' > %t/lit.cfg
RUN: %{lit} -v --show-all %t.txt | FileCheck %s

# LITCFG: lit config lines go here.

# LITRUN: echo XX %if feature {YY} ZZ
# CHECK-NEXT: 'RUN: at line 32'; echo XX YY ZZ
# LITRUN: ...
# CHECK-NEXT: ...
asavonic updated this revision to Diff 418965.Mar 29 2022, 1:15 PM
  • Removed extra line breaks at EOF.
asavonic added inline comments.Mar 29 2022, 1:23 PM
llvm/utils/lit/tests/shtest-if-else.py
2
RUN: grep LITRUN %s | sed -e 's/LITRUN/RUN/' > %t/test.txt
RUN: grep LITCFG %s | sed -e 's/# LITCFG://' > %t/lit.cfg

I wonder if grep and sed are available (and required) for windows builds. Probably not.
In any case, I don't like this separation between inputs and checks either, so I'll try to figure out the way to combine them in one file.

probinson added inline comments.Mar 29 2022, 2:30 PM
llvm/utils/lit/tests/shtest-if-else.py
2

You probably want split-file which is an LLVM-built tool.

tra added inline comments.Mar 29 2022, 2:53 PM
llvm/utils/lit/tests/shtest-if-else.py
2

We can also use %python and run this test to write out the files we need.

asavonic added inline comments.Mar 30 2022, 12:33 PM
llvm/utils/lit/tests/shtest-if-else.py
2

We can easily combine CHECK and RUN lines if we "escape" RUN: in the patterns by using a regex. I decided to keep the lit.cfgfile separate to keep things simple. Otherwise we'll have to deal with test discovery and split-file.

asavonic updated this revision to Diff 419244.Mar 30 2022, 12:34 PM
  • Combined RUN and CHECK lines into a single file.
tra added inline comments.Mar 30 2022, 1:11 PM
llvm/utils/lit/tests/shtest-if-else.py
2

Nice. It's so much easier to follow.

tra accepted this revision.Apr 4 2022, 11:32 AM

LGTM as a potential end user of this feature.
Please wait for OK from one of the LIT owners.

This revision is now accepted and ready to land.Apr 4 2022, 11:32 AM
jdenny requested changes to this revision.Apr 4 2022, 5:18 PM

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

llvm/utils/lit/lit/TestRunner.py
54

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.

1308

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.

1320

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.

llvm/utils/lit/tests/Inputs/shtest-if-else/test.txt
8

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.

52

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.

This revision now requires changes to proceed.Apr 4 2022, 5:18 PM
asavonic updated this revision to Diff 421799.Apr 10 2022, 11:54 AM
asavonic edited the summary of this revision. (Show Details)
  • Reimplemented %if..%else parser to support nested expressions.
  • Added --match-full-lines to the LIT test; added more test cases.
llvm/utils/lit/lit/TestRunner.py
54

Thanks! Fixed the review summary.

1308

Thank you, that makes sense. Moved %if..%else processing before other substitutions.

1320

Right, spaces are no longer required in the last revision of the patch.

llvm/utils/lit/tests/Inputs/shtest-if-else/test.txt
8

Done.

52

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.

MaskRay added inline comments.
llvm/utils/lit/lit/TestRunner.py
1302

It's conventional to use tuples instead of lists to represent immutable objects (stack elements here).

asavonic added inline comments.Apr 10 2022, 2:49 PM
llvm/utils/lit/lit/TestRunner.py
1302

Expression lists are mutable. When we pop the stack, all items are evaluated and the resulting value is appended to the last expression on the previous stack level. For example:

foo-%if feature { bar }
                      ^~~ cursor
stack: [ ['foo-', [token_if, 'feature']],
         [' bar '] ]

When we reach the closing brace, 'bar' is poped out and appended to the 'if' expression:

stack: [ ['foo-', [token_if, 'feature', ' bar ']] ]

Please don't forget to also update the llvm/utils/update_*_checks.py.

Please don't forget to also update the llvm/utils/update_*_checks.py.

It seems that update_*_checks.py scripts are completely independent of lit.py, so there is no way to easily reuse code between them. Parsing of RUN lines and standard substitutions is duplicated there. We have to either integrate these scripts, or copy paste the code.

@asavonic: Thanks for continuing to work on this feature!

Please don't forget to also update the llvm/utils/update_*_checks.py.

It seems that update_*_checks.py scripts are completely independent of lit.py, so there is no way to easily reuse code between them. Parsing of RUN lines and standard substitutions is duplicated there. We have to either integrate these scripts, or copy paste the code.

Has anyone said they need %if in those scripts? Upon seeing %if in a RUN line, those scripts could probably be adjusted to simply complain that %if is not yet supported there.

llvm/utils/lit/lit/TestRunner.py
1213–1214

Can we make the name more specific now that there's also an escapeBraces? Maybe escapePercents? That might be better even if escapeBraces goes away while addressing other issues.

1275
1307

Instead of a loop with a custom stack data structure, a recursive descent parser might be easier to understand and extend, especially if we add more constructs similar to %if later. That is, the python call stack would be used as the parser stack, and stack entries would be simple local variables, function parameters, and function returns.

This is just a suggestion. I'm not insisting. However, I do invite other reviewers to offer their opinion.

1318

Why does %% need to be escaped again here?

1347
llvm/utils/lit/tests/Inputs/shtest-if-else/test.txt
76

If we're going to have these %{ and %} escapes, they need to be documented somewhere. However, I think they're going to be confusing:

  • First, %{foobar} is already a common syntax for lit substitutions. The if-then-else syntax's %{ and } collide with it. For example, how should I write %{pathsep} in a then or else block? I have to write it as %%{pathsep%}, right?
  • More generally, whether { and } need to be escaped and whether %{ and %} are defined depend on the context. That seems error-prone.

What if we go the other way? That is, we could change the { and } around a then or else block to %{ and %}. That way, the entire if-then-else syntax is clearly at the preprocessor/substitution level: %if, %else, %{, and %}. On the other hand, { and } behave as usual in the shell code, and the only character in shell code that ever needs to be escaped continues to be %.

But does that syntax have collisions?

  • I just tried git grep 'RUN.*%}' and found no matches in llvm's repo. It seems %} is unambiguous. If a use arises, it can be escaped as %%}. Again, only % ever has to be escaped.
  • The %{ is really just for symmetry with %}. The %{ doesn't actually need to be recognized in arbitrary shell code anywhere. That is, the if-then-else parser would only need to recognize it after an %if cond or %else. There doesn't appear to be a way for it to collide with, for example, %{pathsep}.

Did I miss anything?

Please don't forget to also update the llvm/utils/update_*_checks.py.

It seems that update_*_checks.py scripts are completely independent of lit.py, so there is no way to easily reuse code between them. Parsing of RUN lines and standard substitutions is duplicated there. We have to either integrate these scripts, or copy paste the code.

I had some code in the downstream CHERI LLVM to expand substitutions using lit. The downside is that the update scripts need to know the path to the build dir to get all the lit config files, but if there is interest I'd be happy to upstream that change.

asavonic updated this revision to Diff 423379.Apr 18 2022, 7:10 AM
asavonic marked an inline comment as done.
asavonic edited the summary of this revision. (Show Details)
  • Reimplemented the parser as a recursive function
  • Changed syntax: %if cond { branch_if } %else { branch_else } to %if cond %{ branch_if %} %else %{ branch_else %}

Please don't forget to also update the llvm/utils/update_*_checks.py.

It seems that update_*_checks.py scripts are completely independent of lit.py, so there is no way to easily reuse code between them. Parsing of RUN lines and standard substitutions is duplicated there. We have to either integrate these scripts, or copy paste the code.

I had some code in the downstream CHERI LLVM to expand substitutions using lit. The downside is that the update scripts need to know the path to the build dir to get all the lit config files, but if there is interest I'd be happy to upstream that change.

@arichardson, I think it makes sense as an optional feature. Right now there is no way to handle substitutions defined in lit.cfg or share the code with lit.py.

llvm/utils/lit/lit/TestRunner.py
1213–1214

Done. escape is now renamed to escapePercents.

1307

Done. It does look better as a recursive function.

1318

This is because of recursive substitution:

substitutions = [
  ("%rec1, "%%s"),
  ("%s", "/path")
]

If we expand %rec1 to %%s in the first iteration of the loop, we have to escape %% before the next iteration. Otherwise instead of of %s we'll get %/path. This is tested in shtest-recursive-substitution/escaping test.

llvm/utils/lit/tests/Inputs/shtest-if-else/test.txt
76

Done. The patch now uses %{ and %} as begin/end tokens. It also helps to distinguish them from boolean expressions with a regex-match ({{regex}}).

While this is not a huge amount of code to be added to lit, I do wonder if we really need it. Wouldn't something like %feature{foo} that expands to 0 or 1 be sufficient? You could then use shell conditionals/test to avoid running lines.
Looking at the examples from D121727, you could do the following:

RUN: if [ %feature{ptxas} -ne 0 ]; then ptxas -c %t-nvptx.ptx -o /dev/null; fi
RUN: test %feature{ptxas} -ne 0 && ptxas -c %t-nvptx.ptx -o /dev/null

Or alternatively if %feature expands to an empty string ("") or "1":

RUN: if [ -n %feature{ptxas} ]; then ptxas -c %t-nvptx.ptx -o /dev/null; fi
RUN: test -n %feature{ptxas} && ptxas -c %t-nvptx.ptx -o /dev/null

Depending on how you set up the ptxas substitution it might even be possible without a %feature{} lit substitution.

While this is not a huge amount of code to be added to lit, I do wonder if we really need it. Wouldn't something like %feature{foo} that expands to 0 or 1 be sufficient? You could then use shell conditionals/test to avoid running lines.
Looking at the examples from D121727, you could do the following:

RUN: if [ %feature{ptxas} -ne 0 ]; then ptxas -c %t-nvptx.ptx -o /dev/null; fi
RUN: test %feature{ptxas} -ne 0 && ptxas -c %t-nvptx.ptx -o /dev/null

The variant with if is probably not going to work on windows.
As for the test - I'm not sure how it works in this case. If test returns a non-zero exit code, then LIT should report an error for the whole RUN line, right?
It is also not clear if we can use more complicated binary expression like test 1 -eq 1 && ( test 2 -eq 2 || test 3 -eq 3 ).
For ptxas we also need to support nested expressions:

; The -arch option is only needed for ptxas < 11.0
; RUN: %if ptxas %{ %ptxas-verify %if !ptxas-11.0 %{-arch=sm_30%} %t.ptx %}

While this is not a huge amount of code to be added to lit, I do wonder if we really need it. Wouldn't something like %feature{foo} that expands to 0 or 1 be sufficient? You could then use shell conditionals/test to avoid running lines.
Looking at the examples from D121727, you could do the following:

RUN: if [ %feature{ptxas} -ne 0 ]; then ptxas -c %t-nvptx.ptx -o /dev/null; fi
RUN: test %feature{ptxas} -ne 0 && ptxas -c %t-nvptx.ptx -o /dev/null

The variant with if is probably not going to work on windows.

Agreed. It won't work in lit's internal shell currently either. %if is a portable if. Maybe it would be good to point this out in the documentation?

As for the test - I'm not sure how it works in this case.

My hunch is that test is also not portable.

llvm/utils/lit/lit/TestRunner.py
1235

It would be nice to extend the test suite to check that these syntax errors are actually detected.

1307

Nice. Thanks.

1318

Make sense. Thanks for explaining.

llvm/utils/lit/tests/Inputs/shtest-if-else/test.txt
22

Maybe test the preservation of space in %else as well.

76

Thanks. Please add a test that a substitution like %{foo} works in the then and else blocks. You'll probably have to define your own as I'm not sure it's safe to rely on the way the built-in substitutions expand.

jdenny accepted this revision.Apr 19 2022, 3:47 PM

I've made a few requests for minor test and doc extensions, but otherwise this LGTM.

Given the recent syntax changes, please give other reviewers a couple more days to comment before landing this.

Thanks for all your work.

This revision is now accepted and ready to land.Apr 19 2022, 3:47 PM
asavonic updated this revision to Diff 424146.Apr 21 2022, 4:18 AM
asavonic edited the summary of this revision. (Show Details)
  • Added negative tests for parsing errors.
  • Added tests for spaces in an %else block, tests with %{sub} substitution.

I've made a few requests for minor test and doc extensions, but otherwise this LGTM.

Given the recent syntax changes, please give other reviewers a couple more days to comment before landing this.

Thanks for all your work.

Thanks a lot! We can wait at least until the next week. D121727 is not ready anyway.

This revision was landed with ongoing or failed builds.Apr 27 2022, 10:30 AM
This revision was automatically updated to reflect the committed changes.