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

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

jdenny added inline comments.


================
Comment at: llvm/utils/lit/lit/TestRunner.py:1223-1224
+    """
+    Common interface for shell commands ('RUN:' lines) and other lit directives
+    that must be processed in their original order as part of a single script.
+
----------------
awarzynski wrote:
> Through this class you are effectively re-introducing the notion of "script directives" in LIT, but this class is only meant for `RUN`, `DEFINE` and `REDEFINE`, right? What about `XFAIL`, `REQUIRES` or `UNSUPPORTED`?
> 
> If this is only meant for a sub-set of LIT directives, I suggest:
> * renaming the class as e.g. `OrderedClassDirective` (so that it is clear that this does not represent //any// LIT directive)
> * adding an assert in the constructor to make sure that `self.keyword` is one of `RUN`, `DEFINE` or `REDEFINE`.
> 
> An assert will document the code in a way that cannot be ignored by developers who decide to extend this class later ;-)
> Through this class you are effectively re-introducing the notion of "script directives" in LIT, but this class is only meant for `RUN`, `DEFINE` and `REDEFINE`, right? What about `XFAIL`, `REQUIRES` or `UNSUPPORTED`?

Thanks for asking this.  I agree the terminology here is confusing.

Even without this patch, "script" is overloaded to mean *at least* three different things in TestRunner.py:

1. The original integrated test script that has all the directives.  For example, `parseIntegratedTestScriptCommands`.
2. The script containing the RUN directives extracted from 1.  For example, the `script` variable in `_parseKeywords`, the `require_script` parameter to `_parseKeywords` and `parseIntegratedTestScript`, the `script` parameter to `applySubstitutions`.
3. The shell script produced after expanding substitutions in 2.  For example, `executeScriptInternal`, `executeScript`.

With this patch, 2 also includes DEFINE/REDEFINE directives.  I chose ScriptDirective to refer to directives in 2 because that's the usage of "script" I saw most often in the code I was modifying.

Should we try to eliminate this overload generally in TestRunner.py?  Here are some possible names:

1. IntegratedTestScript
2. UnexpandedScript
3. ShellScript

Maybe there are better names.  What do you think?

> If this is only meant for a sub-set of LIT directives, I suggest:
> * renaming the class as e.g. `OrderedClassDirective` (so that it is clear that this does not represent //any// LIT directive)

I'd like to pick something that will fit the name we give script 2 above.  If script 2 is OrderedScript, then OrderedDirective works.  However, script 3 is also ordered.  Script 1 is ordered even though the order doesn't matter for some parts.  I think we need something more distinct.  Whatever names we choose, I'll be sure to improve my python documentation accordingly (e.g., don't emphasize order so much).

> * adding an assert in the constructor to make sure that `self.keyword` is one of `RUN`, `DEFINE` or `REDEFINE`.
> 
> An assert will document the code in a way that cannot be ignored by developers who decide to extend this class later ;-)

Do you want asserts for the CommandDirective and SubstDirective constructors as well?  Of course, SubstDirective.adjust_substitutions already asserts about keywords because its behavior is already known to vary based on the keyword.


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

https://reviews.llvm.org/D132513