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

[Andrzej Warzynski via Phabricator via llvm-commits]

awarzynski added inline comments.


================
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:
> 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?
In general, I feel that adding "test/"  might be a bit confusing. What about test sub-directories like "<project-root-dir>/test/Lowering/" in which case "Lowering/lit.local.cfg" could be more accurate than "test/lit.local.cfg". That's why I would drop "test/" in "test/lit.local.cfg". If you do want to keep it, then ...

>  Maybe we should add a corresponding hint for lit.local.cfg, as in my above suggestion. 

+ 1


================
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
----------------
jdenny wrote:
> 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"?
Yes, "even if ``recursiveExpansionLimit``". I was hoping that Phabricator would highlight it.


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

https://reviews.llvm.org/D132513