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

[Louis Dionne via Phabricator via llvm-commits]

ldionne added a comment.

> There should be some user-level documentation about this new feature. It looks like the appropriate place is:

Thanks! Indeed, I wanted to write some documentation but I couldn't find where to add it. I'll do that.



================
Comment at: llvm/utils/lit/lit/BooleanExpression.py:184
         self.assertTrue(BooleanExpression.evaluate('-os', {}, triple))
         self.assertFalse(BooleanExpression.evaluate('arch-os', {}, triple))
 
----------------
jdenny wrote:
> Should there be some minimal test somewhere (maybe here) ensuring that a regex is handled as a literal string when matching triples?  I think that means it cannot ever match (due to `{{` and `}}`), so maybe it's not a very interesting behavior, but we still might want to be aware if we accidentally change it.
Hmm, I agree, I had not considered that. Added two tests: one that a triple doesn't match even if the regex would match, and one where the triple does match the regex when it is treated literally.

The second test is not something we can even encounter in real life, as you say, because triples can't contain special characters like `{{`, but I think the test still has value since it pins down the behavior.

Note: In the future, I would love to remove any notion of a triple in Lit, since I think the special substring handling was its only purpose. If we ever do that, this whole test will become moot.


================
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:
> ldionne wrote:
> > jdenny wrote:
> > > Shouldn't this mention `{{` as a possibility?
> > I'll say `expected '!' or '(' or match-expression` instead, LMK if that's not satisfying.
> While that does reflect the implementation, I think `expected '!', '(', '{{', or identifier` would be more meaningful to a user who isn't familiar with the internal grammar symbol names.  As far as they know, `match-expression` could be, for example, the start symbol for the whole grammar.  In contrast, `{{` and `identifier` are probably clear enough to any user.
Ok, I agree, that seems better. Changed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104572