[PATCH] D76178: [lit] Recursively apply substitutions

[Louis Dionne via Phabricator via llvm-commits]

ldionne added inline comments.


================
Comment at: llvm/utils/lit/lit/LitConfig.py:30
+                 echo_all_commands = False,
+                 recursiveExpansionLimit = None):
         # The name of the test runner.
----------------
delcypher wrote:
> Maybe make the default `0` so it's clearer what the type is supposed to be?
`None` and `0` are different -- `None` means no recursive substitution is attempted, and `0` means that recursive substitution is attempted up to 0 steps. But in the `0` case, it's still an error if there are unexpanded substitutions left after 0 recursive steps.


================
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)" % \
----------------
delcypher wrote:
> 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.
No, we don't lose the results of the other tests. Throwing an exception here results in the test being `UNRESOLVED`, which I believe is what we want.


================
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:
----------------
delcypher wrote:
> 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?
Disallowed now!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76178