[PATCH] D104572: [lit] Add the ability to parse regexes in Lit boolean expressions
Joel E. Denny via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Jun 29 08:42:39 PDT 2021
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
More information about the llvm-commits
mailing list