[libcxx-commits] [libcxx] cd7f975 - [libc++] NFC: Simplify substitutions by using lit recursive substitutions

Dan Liew via libcxx-commits libcxx-commits at lists.llvm.org
Tue Mar 31 22:35:18 PDT 2020

> Thanks for your reply. So your opinion is that my proposed (2) is the right way forward. I still (weakly) think that we made the right design choice because whether to recursively expand or not seems like a property or the test suite as a whole, and not really something that you'd want to customize on a per-directory basis. It's kind of like the timeout time on tests or whether to be verbose.

But our closet proxy for "test suite as a whole" are the TestingConfig objects (they are per test suite directory). Putting the property in LitConfig was never the right design (again sorry I didn't spot this) because that is global to all test suites which is wrong. Given that a test suite can customise its substitutions on a per-directory basis it makes sense that it would be technically possible to configure the substitution recursive expansion on a per-directory basis too. I'm not saying doing that would be a good idea but allowing this is at least consistent with what we already allow users of llvm-lit to do.

It's not like the test timeout or the verbosity. Both of those are properties global to the lit invocation and thus global across all test suite. In both cases it's this way **because** these properties can be set by a command line flag that is global over the lit invocation. When I implemented per-test-timeout, the fact I added it to the LitConfig object rather than TestingConfig was a design mistake that I didn't realise I had made until we got into this discussion.

> It seems to me like the root cause really is that we can modify the LitConfig globally from subdirectories. Consider for example the timeout time -- imagine what would happen if libc++ set the timeout time to e.g. 2 minutes because <reasons>. Then, imagine that the Clang config sets the timeout time to 10 seconds, because it doesn't make sense for Clang tests to take longer than that. In the current state of things, whichever lit.cfg file is processed last will win, which seems brittle. I believe we should either not share the global LitConfig instance (by copying it when we process subdirectories), or make it read-only after construction to make it clear that it shouldn't be modified. And if it was made read-only, then I agree it would make sense to move `recursiveExpansionLimit` to TestingConfig instead.
> What do you think?

As I said above. When I implemented lit's per test timeout I made a design mistake putting it in the LitConfig object. It should be in TestingConfig. I think it would be extremely risky to move to a model where LitConfig objects get copied because there might be existing lit configs that rely on this behaviour.


-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/libcxx-commits/attachments/20200331/c8faaf0a/attachment-0001.html>

More information about the libcxx-commits mailing list