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

[Andrzej Warzynski via Phabricator via llvm-commits]

awarzynski added a comment.

Thanks for adding comments in the Python script, that helps a lot! I've only skimmed through for now and have one small comment about the overall approach. I will try to take a look again later today.



================
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.
+
----------------
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 ;-)


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

https://reviews.llvm.org/D132513