[PATCH] D76178: [lit] Recursively apply substitutions

[Dan Liew via Phabricator via llvm-commits]

delcypher added inline comments.


================
Comment at: llvm/docs/CommandGuide/lit.rst:461
+However, if the ``recursiveExpansionLimit`` property of the ``LitConfig`` is
+set, substitutions will be expanded recursively until that limit is reached.
+
----------------
Maybe mention what it can be set to (i.e. valid range?)


================
Comment at: llvm/utils/lit/lit/LitConfig.py:30
+                 echo_all_commands = False,
+                 recursiveExpansionLimit = None):
         # The name of the test runner.
----------------
Maybe make the default `0` so it's clearer what the type is supposed to be?


================
Comment at: llvm/utils/lit/lit/LitConfig.py:70
         self.echo_all_commands = echo_all_commands
+        self.recursiveExpansionLimit = recursiveExpansionLimit
 
----------------
Maybe use an `@property` wrapper like for `maxIndividualTestTime` and do some validation in the setter so using an incorrect type or value is caught quickly.


================
Comment at: llvm/utils/lit/lit/TestRunner.py:1189
+        if processed != ln:
+            raise ValueError("Recursive substitution of '%s' did not complete "
+                             "in the provided recursion limit (%s)" % \
----------------
I don't think we should raise an exception here because I think that means we'd loose the results of all other tests. We should fail the test with an appropriate message.


================
Comment at: llvm/utils/lit/tests/unit/TestRunner.py:251
+                         ("%rec3", "%rec2"), ("%rec4", "%rec3"), ("%rec5", "%rec4")]
+        for limit in [-1, 0, 1, 2, 3, 4]:
+            try:
----------------
What does `-1` as a limit mean? If I've read the code correctly any value `<= 0` will have the same effect so why even allow negative values?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76178