[PATCH] D76178: [lit] Recursively apply substitutions

[Dan Liew via Phabricator via llvm-commits]

delcypher added a comment.

@ldionne Thanks for addressing my comments. The patch looks like its in very good shape. I have a minor nit about the documentation. Other than that LGTM.



================
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.
+
----------------
delcypher wrote:
> Maybe mention what it can be set to (i.e. valid range?)
Thanks. This documents what happens if `recursiveExpansionLimit > 0`. However we also allow `recursiveExpansionLimit == 0`. As you pointed out there is a difference between `recursiveExpansionLimit` being `None` and being equal to `0`. Could you document that here?


================
Comment at: llvm/utils/lit/lit/LitConfig.py:70
         self.echo_all_commands = echo_all_commands
+        self.recursiveExpansionLimit = recursiveExpansionLimit
 
----------------
delcypher wrote:
> 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.
Thanks for fixing this.


================
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)" % \
----------------
ldionne wrote:
> 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.
Ah I didn't realise that. Now that I've looked closer I see you have a test for this. Awesome!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76178