[PATCH] D76178: [lit] Recursively apply substitutions

Dan Liew via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Sat Mar 14 11:15:58 PDT 2020


delcypher added a comment.

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.
- Debugging lit substitutions becomes really difficult. Right now I can add a few print statements in the lit.site.cfg files to print out the current substitutions (or ones that interest me) to see what they expand to. With this change that doesn't work anymore because the full expansion happens much later.

The way most test suites handle achieving your end goal is to just have variables that represent the different pieces of a command line and then define the substitution appropriately. E.g.

  options = { 
    'asan': '-fsanitize=address',
    'warnings': '-Wall',
    'opt':  '-O0',
    'debug': '-g',
    'cc': '/usr/bin/clang',
  }
  
  cc_asan_subst = '{cc} {warnings} {asan} {opt} {debug}'.format(**options)
  print(cc_asan_subst)
  cc_subst = '{cc} {warnings}  {opt} {debug}'.format(**options)
  print(cc_subst)

While not quite as elegant as what this patch proposes it is so much easier to debug. Are you trying to do something that is difficult to express in the `lit.site.cfg` files?



================
Comment at: llvm/utils/lit/lit/TestRunner.py:1178
+        processed = processLine(ln)
+        while processed != ln:
+            ln = processed
----------------
If `processed` eventually expands to substitution already processed (e.g. a substitution expands to something containing itself) then this will never terminate. If we are going to have this feature then we should have a check for substitution cycles somewhere because this will just look like a lit hang otherwise.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76178





More information about the llvm-commits mailing list