[PATCH] D76178: [lit] Recursively apply substitutions

[Louis Dionne via Phabricator via llvm-commits]

ldionne added a comment.

In D76178#1928073 <https://reviews.llvm.org/D76178#1928073>, @delcypher wrote:

> @ldionne
>
> > In D76178#1922990 <https://reviews.llvm.org/D76178#1922990>, @delcypher wrote:
> > 
> >> While I can see the appeal of this I need convincing that this actually needed. I see too many downsides
> >>
> >> - Possibility of non-termination being introduced that we'll have to check for which is a cost that will have to be paid every time we execute a test.
> > 
> > 
> > I agree we should check for this, however it's not clear to me this is going to make a noticeable perf difference in practice. The tests we run involve executing processes and doing things that are a lot more expensive than the simple lit substitution, even if we check for  non-terminating substitutions. I think it should be possible to make it fairly quick in the usual case where full substitution happens after exactly one substitution.
>
> Testing **only** for the "usual case" isn't sufficient to cover all situations where infinite recursion might be introduced. It looks lit actually supports regex substitutions (which I didn't even know it supported). That makes checking for cycles even harder. Maybe a low default recursion depth would be the solution here?


Yeah, a low recursion maximum also appears like the simplest and cleanest solution to me. Concretely, I think it should rarely happen, and it's always obviously a programmer error in the `lit.cfg`. A max recursion depth would ensure that the programmer finds that problem quickly and simply.

> Doing this is a very useful way to inspect what `%clang` will expand to.
> 
> Your change will break this because now people can do something like this in the their lit config.
> 
>   config.substitutions.add( ('%clang', '%cc %arch %warnings %sysroot'))

[Sorry, the first shot was sent with some keyboard shortcut when I was right here]

I frankly think that this should be for the author of the `lit.cfg` to handle, not for `lit` to enforce by limiting its functionality. But as you say below, I agree lit could have a convenience API to expand substitutions to allow people to print them.

> Side note: If you're rewriting the test suite now would be a good opportunity to avoid a mistake made in the past. Lit substitutions don't handle the case where a substitution is a prefix of another (e.g. `%clang` and `%clang_asan`, the resolution is dependent on the order the substitutions are defined).

Nice, I didn't know that. I don't get to redefine the substitutions, however, cause those are the ones of the existing libc++ test suite.

> Why do you need to create your own test format to achieve your goals? Everything you've described so far is achievable with lit and a suitable `lit.cfg` file today.

I don't think it is possible to inject custom commands in front of a `.sh.cpp` without creating a custom test format. But regardless, a custom test format is a nice place to encapsulate that logic.

> I'm not entirely convinced by your use case but I do see the appeal of the feature. What I want is to make sure this feature is not added in a form where
> 
> - It's easy to shoot yourself in the foot (with infinite recursion).

A max recursion depth will handle that.

> - It's very difficult to debug when things go wrong.

I'll look into providing a convenience API to print expanded substitutions, but even then I don't believe this should be sufficient to hold off the change, since you can always avoid using the feature if for some reason you find it too complicated.

> - The feature isn't documented.

Where can I document it? I'm happy to do that.

> If you can fix those problems then I'm not opposed to this. My suggestions would be.

I'll look into those, thanks for the feedback!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76178