[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:
> jdenny wrote:
> > 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.
> **GENERAL DISCUSSION**
> 
> > Thanks for asking this. I agree the terminology here is confusing.
> 
> This is way deeper than I expected, thanks for the thorough explanation!
> 
> > Should we try to eliminate this overload generally in TestRunner.py?
> 
> Yes, but probably in a different patch. This one is already quite long. Below are some of my thoughts based on your explanation.
> 
> > Here are some possible names:
> >
> >   IntegratedTestScript
> >   UnexpandedScript
> >   ShellScript
> >
> > Maybe there are better names. What do you think?
> 
> All in all, I would consider doing some refactoring it in two steps (based on my understanding, which might be incorrect):
> 1. Clearly define what LIT directives are and what directives are supported  (i.e. `RUN`, `XFAIL`, `REQUIRES` ...) . Ideally, every class that manipulates them should clearly define what directives it will process. In the case of `ScriptDirective`, it's `RUN`, `DEFINE` and `REDEFINE`, right? 
> 2. Hopefully, Step 1. will be a good source of inspiration for renaming things. That's when I would try to fix what "script" means in various classes. 
> 
> I'm suggesting Step 1. as from your description 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. 
> 
> As for Step 2., how about the following classification:
> 
> * `RawScript` (base class),
> * `ExpandableScript` (specialization of `RawScript`, with substitutions either to be expanded or already expanded).
> 
> **RENAMING**
> 
> The refactoring discussed above should happen in a separate patch IMO. In this patch I would just focus on renaming `ScriptDirective`.
> 
> > 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. 
> 
> So what's **unique** about script 2 then? From your description, it sounds like `OrderedScriptWithSubstitutions`. Or something similar.
> 
> > Do you want asserts for the CommandDirective and SubstDirective constructors as well? 
> 
> Yes, it sounds like these should be only used with specific directives and IMO that should be very clear from the implementation.
> 
> > Of course, SubstDirective.adjust_substitutions already asserts about keywords
> 
> True, but that's quite far from object creation. To me it sounds like `SubstDirective` should only work for specific directives. You can be strict about it and enforce it during object creation (which sends very clear message to class users and/or other developers) or defer it until that assumption becomes vital. Both approaches will work. I'm merely suggesting to express the intent more explicitly.
> **GENERAL DISCUSSION**

> > Should we try to eliminate this overload generally in TestRunner.py?
> 
> Yes, but probably in a different patch.

Agreed.

> All in all, I would consider doing some refactoring it in two steps (based on my understanding, which might be incorrect):

I agree that steps like these would improve TestRunner.py.  Implementing them myself is more than I want to sign up for right now, but I would be happy to review patches.  I would like to finish the DEFINE/REDEFINE series first though.

> In the case of `ScriptDirective`, it's `RUN`, `DEFINE` and `REDEFINE`, right? 

Right.

> 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.  The first two script forms I identified (the original test file, and the one with only RUN/DEFINE/REDEFINE directives) both have to distinguish directives by the directive keyword.  Only the third script form has just shell commands and thus no directive keywords.

> As for Step 2., how about the following classification:
> 
> * `RawScript` (base class),

Might be fine.  We could discuss more in a separate patch to implement the refactoring.

> * `ExpandableScript` (specialization of `RawScript`, with substitutions either to be expanded or already expanded).

`ExpandableScript` works for me.

> **RENAMING**
> 
> The refactoring discussed above should happen in a separate patch IMO. In this patch I would just focus on renaming `ScriptDirective`.

Agreed.

> > 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. 
> 
> So what's **unique** about script 2 then? From your description, it sounds like `OrderedScriptWithSubstitutions`. Or something similar.

I need to think more about whether the "ordered" property is really the right emphasis here.  In my mind, any "script" is an ordered sequence, and google just showed me definitions of "script" that agree, so `OrderedScript` sounds redundant.  Moreover, maybe directives not included in this script form will one day (or already do?) care about order in some way I haven't imagined.

You suggested `ExpandableScript` above.  A consistent name for the directive would then be `ExpandableScriptDirective`.  I think that succinctly captures which stage of the transformation this script and its directives represents.  I went ahead and implemented that and updated the python documentation to reflect that name.

Maybe there are better names.  I think we both have suggested there's a larger discussion and mostly orthogonal patch to work on, and the naming could be revisited then.  Is that ok for now?

> > Do you want asserts for the CommandDirective and SubstDirective constructors as well? 
> 
> Yes, it sounds like these should be only used with specific directives and IMO that should be very clear from the implementation.

I tried, but a test failure reminded me that some test suites define their own keywords in place of `RUN:`.  See `llvm/utils/lit/tests/unit/TestRunner.py`.  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.

In other words, we need to go the other way: less strictness about keywords.

I implemented that by extending `SubstDirective` with a new field indicating whether it defines or redefines.  Now, `adjust_substitutions` doesn't need to check the directive's keyword at all.

I also minimally extended `llvm/utils/lit/tests/unit/TestRunner.py` to check that custom keywords for `DEFINE:` and `REDEFINE:` work as expected.  I could probably test much more here, but I haven't found much evidence that keyword customization is used much, so I'm not sure exhaustive testing is worthwhile.


================
Comment at: llvm/utils/lit/lit/TestRunner.py:1388
+            return
+        assert self.keyword == 'REDEFINE:'
+        if len(existing) > 1:
----------------
awarzynski wrote:
> I would move this assert to the constructor. In here you can add something like this instead:
> ```
> if self.keyword != 'REDEFINE:':
>   raise ValueError(f"Unsupported directive {self.keyword})")
> ```
I've removed the assert entirely, as discussed in the other comment.


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

https://reviews.llvm.org/D132513