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

[Joel E. Denny via Phabricator via llvm-commits]

jdenny accepted this revision.
jdenny added a comment.
This revision is now accepted and ready to land.

LGTM.

I apologize if this is frustrating, but can you wait a couple of days to commit?  I'd like to give other reviewers just a bit more time to comment in case we overlooked something.



================
Comment at: llvm/docs/TestingGuide.rst:465-468
+- Any Python regular expression enclosed in ``{{ }}``, in which case the boolean
+  expression is satisfied if any feature matches the regular expression. Also note
+  that regular expressions can appear inside an identifier, so for example
+  ``he{{l+}}o`` would match ``helo``, ``hello``, ``helllo``, and so on.
----------------
Thanks for adding this.

Tiny nit: The second bullet is a different animal than the other two.  I'd put the other two next to each other or integrate them.  But it's no big deal if you prefer it as is.


================
Comment at: llvm/utils/lit/lit/BooleanExpression.py:184
         self.assertTrue(BooleanExpression.evaluate('-os', {}, triple))
         self.assertFalse(BooleanExpression.evaluate('arch-os', {}, triple))
 
----------------
ldionne wrote:
> 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.
That all makes sense to me.  Thanks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104572