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

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

jdenny added inline comments.


================
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
----------------
awarzynski wrote:
> [nit] I'd be tempted to add "(e.g. lit.cfg or lit.local.cfg)" To avoid any potential ambiguity.
There are now several mentions of lit configuration files between here and the original description, which has "(e.g., lit.cfg or lit.local.cfg)".  To me, it seems arbitrary to give examples at this particular mention.  See how it reads to you now.  If you still feel it would be helpful, I'll add it.


================
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`.
----------------
awarzynski wrote:
> "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".
> 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`).

Because this bulleted list is meant to be a general description of substitution behavior, I feel it's better to keep this point here than to complicate the introductory example with it.  However, I did abbreviate the description and append it to the bullet that describes the default single pass over the substitution list, where it seems like a natural fit.

> An example would also be super helpful! You could use the contents of "recursiveExpansionLimit.txt".

Done.  I'm using that to replace the original `recursiveExpansionLimit` documentation.


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

https://reviews.llvm.org/D132513