[PATCH] D76178: [lit] Recursively apply substitutions

[Louis Dionne via Phabricator via llvm-commits]

ldionne added a comment.

In D76178#1950099 <https://reviews.llvm.org/D76178#1950099>, @jdenny wrote:

> Sorry to be late to this review....
>
> > This allows defining substitutions in terms of other substitutions. For
> >  example, a %build substitution could be defined in terms of a %cxx
> >  substitution as '%cxx %s -o %t.exe' and the script would be properly
> >  expanded.
>
> [...]
>
> An advantage of this patch over the existing behavior would appear to be that substitution definition order no longer matters.  Actually, it does matter because changing the order can affect the required value of `recursiveExpansionLimit`.  Generally, test authors shouldn't have to think about the number of levels of recursion.  To ease the maintenance burdern as substitution definitions evolve, perhaps the documentation should say that choosing large values (say, multiples of 100) even when small, more exact values are sufficient is preferable and incurs zero performance penalty (as long as definitions are not infinitely recursive).


You're right that if one sets up substitutions in the correct order (and assuming no circular dependencies), a single pass is sufficient to expand substitutions. However, the problem is precisely that one usually doesn't have control over the order in which substitutions are set up, and frankly I don't think it should matter. In the current state of things, the order in which substitutions are performed is essentially an implementation detail, and I believe it should stay that way unless we can provide a reasonable way for users to control it.

Given that, what we could do instead is switch the implementation from doing multiple passes to doing a topological sort of the substitutions based on whether a substitution is required by another one. The `recursiveExpansionLimit` would then become a boolean like `orderSubstitutionsSomething`. It would be an error if no linearization can be found, i.e. if there's a dependency cycle in substitutions. Note that it would still have to be opt-in, because some test suites use patterns without delimiters and they (almost certainly unknowingly) rely on the fact that their substitutions happen to be processed in a specific order.

I thought about this and I both couldn't find an implementation of topological sort in the Python Standard Library, and also thought that solution was overkill. Please let me know if you think that direction should be explored, and I'll research it some. However, let's not revert this because the feature is useful in its current state and the libc++ test suite needs it -- if we switch to a topo sort the usage will be mostly the same so it'll be easy to change libc++ at the same time.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76178