[PATCH] D132513: [lit] Implement DEFINE and REDEFINE directives

[Joel E. Denny via Phabricator via llvm-commits]

jdenny added a comment.

In D132513#3782848 <https://reviews.llvm.org/D132513#3782848>, @awarzynski wrote:

> Sounds like our mental models are slightly different. This is the breakdown that derived from our discussion:

It sounds like this is just a difference in the way we're using the terminology.  Clear terminology is indeed important, but...

> Also, another patch, another time :)

.... yes, I think it's reasonable to defer.

> Basically, I'm suggesting keeping the discussion on `recursiveExpansionLimit` and how it affects `DEFINE` and `REDEFINE`  separate (perhaps a paragraph on "advanced usage" or "fine details").

Thanks for those comments.  I'll pursue them soon.

> I won't have any more comments after this.

But feel free if something comes up.  Once you've hit accept, I'll likely stall for about a week anyway to give other reviewers a chance to catch up.

> This is a very well written, thoroughly tested and documented patch. Many thanks for all the hard work! It's been a rather long discussion and I really appreciate you seeing this through. This is going to make our testing infrastructure much more powerful and I suspect that LLVM folks will be adopting this in their testing soon :)

Thanks for saying so, and thank you for your review, which has definitely improved the patch.  I appreciate thorough reviews, especially on patches affecting LLVM's testing infrastructure as they can easily impact all LLVM developers.



================
Comment at: llvm/docs/TestingGuide.rst:680
+
+- Lit configuration files (e.g., ``test/lit.cfg`` or ``lit.local.cfg``) can
+  define substitutions for all tests in a test directory.  They do so by
----------------
awarzynski wrote:
> Hm, in practice it's "lit.cfg.py" and "lit.local.cfg.py", but all LIT docs seem to drop the extension 🤔 .
> Hm, in practice it's "lit.cfg.py" and "lit.local.cfg.py", but all LIT docs seem to drop the extension 🤔 .

All four are used.  Lit even permits some customization (see `--config-prefix`).  So these are just common examples.


================
Comment at: llvm/docs/TestingGuide.rst:680
+
+- Lit configuration files (e.g., ``test/lit.cfg`` or ``lit.local.cfg``) can
+  define substitutions for all tests in a test directory.  They do so by
----------------
jdenny wrote:
> awarzynski wrote:
> > Hm, in practice it's "lit.cfg.py" and "lit.local.cfg.py", but all LIT docs seem to drop the extension 🤔 .
> > Hm, in practice it's "lit.cfg.py" and "lit.local.cfg.py", but all LIT docs seem to drop the extension 🤔 .
> 
> All four are used.  Lit even permits some customization (see `--config-prefix`).  So these are just common examples.
You suggested dropping "test/" here.  I assume "test/" was meant to hint that `lit.cfg` is typically at the root directory of a test suite.  Maybe we should add a corresponding hint for `lit.local.cfg`, as in my above suggestion.  Does that help?


================
Comment at: llvm/docs/TestingGuide.rst:769-770
+   confusing expansions.  The new substitution is inserted at the start of the
+   substitution list so that it will expand first.  Thus, even if
+   ``recursiveExpansionLimit`` is not used, this substitution's value can
+   contain any substitution previously defined, whether in the same test file or
----------------
awarzynski wrote:
> [nit] I would remove this part to keep this short and to the point
By "this part", do you mean "even if ``recursiveExpansionLimit`` is not used" or the whole sentence starting with "Thus"?


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

https://reviews.llvm.org/D132513