This is an archive of the discontinued LLVM Phabricator instance.

[ASTMatchers] Add `isConsteval` matcher
ClosedPublic

Authored by Izaron on Jan 18 2022, 2:39 PM.

Details

Summary

Support C++20 consteval functions and C++2b if consteval for AST
Matchers.

Diff Detail

Event Timeline

Izaron requested review of this revision.Jan 18 2022, 2:39 PM
Izaron created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptJan 18 2022, 2:39 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
Izaron added inline comments.Jan 18 2022, 2:42 PM
clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
1807

Actually if consteval is a C++23 feature, but there is no isCXX23orLater() method. The is consteval construction seems to be backported to C++20 in clang, as it compiles and just emits a warning

warning: consteval if is a C++2b extension [-Wc++2b-extensions]

Adding a few more reviewers in case there are differing opinions.

clang/include/clang/ASTMatchers/ASTMatchers.h
5173–5174
5179

It'd be good to show an example of:

if ! consteval {}

if ! consteval {} else {}

as well so users know what to expect.

Should there be a matcher so users can distinguish between if consteval and if ! consteval?

Test cases for these sort of things would also be appreciated.

clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
1794–1796
1807–1809

Yes, we support if consteval as an extension in older language modes, so I think this is reasonable.

Izaron updated this revision to Diff 401363.Jan 19 2022, 12:10 PM

Fixed commit

Izaron updated this revision to Diff 401365.Jan 19 2022, 12:12 PM

Fixed commit (with clang-format)

Izaron marked 4 inline comments as done.Jan 19 2022, 12:23 PM
Izaron added inline comments.
clang/include/clang/ASTMatchers/ASTMatchers.h
5179

Thanks!

Should there be a matcher so users can distinguish between if consteval and if ! consteval?

This is an interesting question. I wonder how intensively if ! consteval is ever going to be used in real life to make a unique matcher for it? I can't make up a hypothetical checker that would need it.

Let's see what other fellow reviewers think about it =)

Izaron marked an inline comment as done.Jan 20 2022, 1:43 AM
cor3ntin added inline comments.Jan 20 2022, 3:29 AM
clang/include/clang/ASTMatchers/ASTMatchers.h
5179

That's a good question.
Maybe we can add it later if there is a need?
I suspect that if we need 2, we need 3, hasConsteval, hasNegatedConsteval, hasNonNegatedConsteval

Until a need present itself, maybe one is enough

cor3ntin accepted this revision.Jan 20 2022, 5:02 AM
This revision is now accepted and ready to land.Jan 20 2022, 5:02 AM
aaron.ballman requested changes to this revision.Jan 20 2022, 5:37 AM
aaron.ballman added inline comments.
clang/include/clang/ASTMatchers/ASTMatchers.h
5179

This is an interesting question. I wonder how intensively if ! consteval is ever going to be used in real life to make a unique matcher for it? I can't make up a hypothetical checker that would need it.

That's not the salient question. I'm more worried that the current interface is confusing and not easy to extend. There's no motivation in the review for why this support is needed, and we tend to err on the side of being conservative with matcher interfaces (we only add them when they're necessary and generally useful, otherwise users can use local matchers).

I think the matcher isn't well designed for IfStmt and we should drop support for that initially, and only focus on support for FunctionDecl where the behavior is more clear. Then we can decide what the correct interface is for IfStmt once we have a better picture of how the matcher is expected to be used and a more general design for it.

This revision now requires changes to proceed.Jan 20 2022, 5:37 AM
cor3ntin added inline comments.Jan 20 2022, 5:53 AM
clang/include/clang/ASTMatchers/ASTMatchers.h
5179

That's not the salient question. I'm more worried that the current interface is confusing and not easy to extend. There's no motivation in the review for why this support is needed, and we tend to err on the side of being conservative with matcher interfaces (we only add them when they're necessary and generally useful, otherwise users can use local matchers).

Arguably, there is one for if constexpr and both of them should be symmetric. Introducing asymmetry for no good reason seems wrong. I assume that there was a motivation initially?

I think the matcher isn't well designed for IfStmt and we should drop support for that initially, and only focus on support for FunctionDecl where the behavior is more clear. Then we can decide what the correct interface is for IfStmt once we have a better picture of how the matcher is expected to be used and a more general design for it.

This was initially my gut feeling, however there is no separate matcher for if constexpr(false) and if constexpr (true), so I don't think that if ! consteval needs to be treated differently here.
Either way, i don't think adding one hurts

aaron.ballman added inline comments.Jan 20 2022, 6:03 AM
clang/include/clang/ASTMatchers/ASTMatchers.h
5179

You can match over the expression in if constexpr to determine whether it's true or false (at least in theory, we probably need some better matchers around constant expression value comparisons), but you cannot match over the syntax of if ! consteval vs if consteval, so I see them as being different cases in that regard.

aaron.ballman accepted this revision.Jan 20 2022, 6:25 AM

LGTM with a minor adjustment to the documentation.

clang/include/clang/ASTMatchers/ASTMatchers.h
5173

Needs to be re-wrapped to 80-col, mostly wanting to call out the if ! consteval bit more explicitly.

5179

I spoke with @cor3ntin off-list a bit about this and am now more comfortable. What was bothering me was that:

if consteval {
}

if ! consteval {
}

are both matched by ifStmt(isConsteval()) and matching on the second case looks downright odd to me. However, that is the interface of our AST. We can add isNonNegatedConsteval() and isNegatedConsteval() as matchers when we need to distinguish between the cases. Thanks for bearing with me while I figured that out. :-)

This revision is now accepted and ready to land.Jan 20 2022, 6:25 AM
Izaron updated this revision to Diff 401679.Jan 20 2022, 9:57 AM

Fixed: make docs more explicit

Izaron marked 6 inline comments as done.Jan 20 2022, 9:58 AM

Thanks! Let's see if docs are more explicit and nicer now.

aaron.ballman accepted this revision.Jan 20 2022, 10:18 AM

LGTM! Do you need someone to commit on your behalf? If so, what name and email address would you like used for patch attribution?

LGTM! Do you need someone to commit on your behalf? If so, what name and email address would you like used for patch attribution?

Sorry, I should have set the Author property of the commit! I would like to use name "Evgeny Shulgin" and email "izaronplatz@gmail.com". Thanks!

LGTM! Do you need someone to commit on your behalf? If so, what name and email address would you like used for patch attribution?

Sorry, I should have set the Author property of the commit! I would like to use name "Evgeny Shulgin" and email "izaronplatz@gmail.com". Thanks!

No worries! I've commit on your behalf in b80db150cdba17b3d4970389025f95b1c93482b8 (I also added a short release note, which I forgot to mention during the review). Thank you for the new matcher!