[PATCH] D104572: [lit] Add the ability to parse regexes in Lit boolean expressions

[Louis Dionne via Phabricator via llvm-commits]

ldionne marked 3 inline comments as done.
ldionne added inline comments.


================
Comment at: llvm/utils/lit/lit/BooleanExpression.py:114
             self.expect(')')
         elif not BooleanExpression.isIdentifier(self.token):
             raise ValueError("expected: '!' or '(' or identifier\nhave: %s" %
----------------
jdenny wrote:
> `isIdentifier` seems misnamed if it accepts `{{`.
> 
> The logic should probably be something like either `not isIdentifier and not isRegexOpen` or `not isIdentiferOrRegexOpen`.
I agree that `isIdentifier` is misnamed, I'll fix that.

I also agree that it should be something like `not isIdentifier and not isRegexOpen`, however this implementation does not track the fact that we're parsing the content of a regular expression -- it doesn't know that it's inside a `{{`. Instead, the tokenization pattern was augmented to treat anything with `{{<whatever>}}` as a token of its own - that was by far the simplest way I could find to implement it. So instead, I believe the fix is to simply rename `isIdentifier` to `isMatchExpression`, and to acknowledge that we now have a new leaf in the grammar, and that `isMatchExpression` basically allows us to detect that.


================
Comment at: llvm/utils/lit/lit/BooleanExpression.py:115
         elif not BooleanExpression.isIdentifier(self.token):
             raise ValueError("expected: '!' or '(' or identifier\nhave: %s" %
                              self.quote(self.token))
----------------
jdenny wrote:
> Shouldn't this mention `{{` as a possibility?
I'll say `expected '!' or '(' or match-expression` instead, LMK if that's not satisfying.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104572