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

[Andrzej Warzynski via Phabricator via llvm-commits]

awarzynski added a comment.

> Implementing them myself is more than I want to sign up for right now

Totally understandable!

>> it sounds like "script" in TestRunner.py refers to the command that follows a directive. At least in it's most general form. That wasn't clear to me at all.
>
> I might be missing your point here, but that description doesn't match my view.

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

  ; RUN: <script>
  
  <test input>
  
  ; <expected output>

To me, `RUN: <script>` is a "LIT directive" followed by a "shell script" (that may or may not be expandable). I feel that refactoring TestRunner.py to be strict about this distinction would make a lot of things easier. But perhaps I'm oversimplifying this? Also, another patch, another time :)

> What if someone wants to do that for DEFINE: or REDEFINE:? I'm not sure whether anyone ever will, but it would be more consistent to permit it.

That sounds like a bit "exotic" use case that complicates a fair few things here, but I agree that consistency is key. Thanks for trying!

Having read and understood the code (thanks for bearing with me and all the extra context), I've skimmed through the documentation again. I've made a couple of minor suggestions, but nothing serious. It reads really well for me. 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").

**Tl;Dr**
I won't have any more comments after this. 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 :)



================
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
----------------
Hm, in practice it's "lit.cfg.py" and "lit.local.cfg.py", but all LIT docs seem to drop the extension 🤔 .


================
Comment at: llvm/docs/TestingGuide.rst:744
+  appearing in its value in order for the latter to expand.
+- While lit configuration files can insert anywhere in the substitution list,
+  the insertion behavior of the ``DEFINE:`` and ``REDEFINE:`` directives is
----------------
[nit] I'd be tempted to add "(e.g. lit.cfg or lit.local.cfg)" To avoid any potential ambiguity.


================
Comment at: llvm/docs/TestingGuide.rst:748-752
+- If you find that the substitution expansion order is confusing or otherwise
+  insufficient for your test suite, consider specifying
+  ``recursiveExpansionLimit`` in a lit configuration file to enable multiple
+  passes through the substitution list, as documented in
+  :doc:`CommandGuide/lit`.
----------------
"confusing" and "insufficient" are a bit ambiguous and vague. Sometimes less is more and I would skip this point. Instead, I would mention above (after "There's a simple way to remember the required definition order in a test file: define a substitution before you refer to it.") that "This behavior can be overridden by specifying `recursiveExpansionLimit`). An example would also be super helpful! You could use the contents of "recursiveExpansionLimit.txt".


================
Comment at: llvm/docs/TestingGuide.rst:753-755
+- Defining a substitution in terms of itself, whether directly or via other
+  substitutions, usually produces an infinitely recursive definition that cannot
+  be fully expanded regardless of ``recursiveExpansionLimit``.  It does *not*
----------------



================
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
----------------
[nit] I would remove this part to keep this short and to the point


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

https://reviews.llvm.org/D132513