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

[Andrzej Warzynski via Phabricator via llvm-commits]

awarzynski 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.
+
----------------
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.


================
Comment at: llvm/utils/lit/lit/TestRunner.py:1388
+            return
+        assert self.keyword == 'REDEFINE:'
+        if len(existing) > 1:
----------------
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})")
```


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

https://reviews.llvm.org/D132513