[PATCH] D76178: [lit] Recursively apply substitutions

[Joel E. Denny via Phabricator via llvm-commits]

jdenny added a comment.

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.

@ldionne : Thanks for working on this.  Howver, as one of the test cases added by this patch suggests, the above example is already supported if `%build` appears earlier in the list of substitutions than `%cxx`.  For example, `llvm/utils/lit/tests/lit.cfg` defines `%{lit}` in terms of `%{python}`.  There's no need to think about infinite recursion or recursion limits.  Instead, you have to think about the search-and-replace order.

Could you please explain a bit why the above behavior is not sufficient for your use case?  I'm not necessarily arguing against this patch, but I'd like to better understand its motivation.

  By default, substitutions are expanded exactly once, so that if e.g. a
  substitution ``%build`` is defined in top of another substitution ``%cxx``,
  ``%build`` will expand to ``%cxx`` textually, not to what ``%cxx`` expands to.
  However, if the ``recursiveExpansionLimit`` property of the ``LitConfig`` is
  set to a non-negative integer, substitutions will be expanded recursively until
  that limit is reached. It is an error if the limit is reached and expanding
  substitutions again would yield a different result.

As a result of the existing behavior, the above documentation is not accurate, and neither is the name `recursiveExpansionLimit`.  That is, if you append substitutions in the order of the recursion levels, `recursiveExpansionLimit=0` permits any number of recursion levels in the substitution definitions.  A name like `expansionPassLimit` would be more accurate.

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76178