[PATCH] D18185: [lit] Allow boolean expressions in REQUIRES and XFAIL and UNSUPPORTED

[Daniel Dunbar via llvm-commits]

ddunbar added a comment.

My comments:

1. I am +1 on the change overall, I definitely think this will be nicer to have in place.

2. It would be good to get documentation for this, in `docs/CommandGuide/lit.rst`. There isn't much description of the "ShTest" format yet, but we should definitely try and improve that.

3. Since the primary clients of lit use "C-family" style boolean operators, I think my preference would be for using || / && / ! / true / false.

4. I find it a bit unfortunate that the semantics for "," aren't the same between XFAIL/UNSUPPORTED and REQUIRES... I'm not sure if this is just so natural that it all works out, or if it would be better to simply require using ||/&& explicitly. Any thoughts here, did you do a survey of what is in use currently? Could we migrate to dropping ','?

5. Would it be worth getting quoted string support? That might give a nice longer term path to having a clean syntax rather than the current identifier regex.

Thanks!


================
Comment at: utils/lit/lit/BooleanExpression.py:45
@@ +44,3 @@
+    def tokenize(string):
+        pattern = re.compile(r'\A\s*([()]|\b[_a-zA-Z][-=:/_a-zA-Z0-9]*)\s*(.*)\Z')
+        while True:
----------------
This can be lifted out to a global constant to save on the regex compile.

================
Comment at: utils/lit/lit/BooleanExpression.py:125
@@ +124,3 @@
+        self.expect(BooleanExpression.END)
+        return self.value
+
----------------
I would write the tests for the basic parser inline here using the unit test style (see ShUtil.py), it is much more convenient and transparent than using lit for this kind of thing. The ShUtil tests themselves are run by the "tests/shell-parsing.py" test.


http://reviews.llvm.org/D18185