This is an archive of the discontinued LLVM Phabricator instance.

[ASTMatchers] Add `isConstinit` matcher
ClosedPublic

Authored by Izaron on Jan 20 2022, 4:30 PM.

Details

Summary

Support C++20 constinit variables for AST Matchers.

Diff Detail

Event Timeline

Izaron requested review of this revision.Jan 20 2022, 4:30 PM
Izaron created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptJan 20 2022, 4:30 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
Izaron added a comment.EditedJan 20 2022, 4:38 PM

I'm going to use this matcher very soon in my new clang-tidy check.

If you're interested and want to read more, I have a design document about my ne check - here (gist.github.com) is the concrete check for constinit variables.

I think an ASTMatcher would be good, because:

  1. As far I tried there is no way to get only constinit variables in clang-query (?)
  2. The way to check in code if a variable is constinit is not quite obvious. A user may expect Node.isConstinit() (because Node.isConstexpr() works), but it's not working. The user has to examine AST and grep until they find Node.hasAttr<ConstInitAttr>().

P. S. If this commit will be accepted, kindly please commit it on my behalf, because I can't do it =) You can use my name Evgeny Shulgin and izaronplatz@gmail.com

Izaron updated this revision to Diff 401817.Jan 20 2022, 4:43 PM

Removed the brackets in { return }

Izaron updated this revision to Diff 401872.Jan 20 2022, 11:26 PM

Fix clang-format

aaron.ballman added inline comments.Jan 21 2022, 12:32 PM
clang/include/clang/ASTMatchers/ASTMatchers.h
5223

This isn't quite correct -- there are two forms of ConstInitAttr:

constinit int i = 0; // We want to match this one
[[clang::require_constant_initialization]] int j = 0; // We don't want to match this one

We typically want AST matchers to model the AST and this one does not... but, I think it is okay. constinit was based on the implementation experience we got with [[clang::require_constant_initialization]] and so we continue to model it as an attribute rather than a bit on the AST node. Having this matcher helps to distinguish the cases (otherwise we could just use the hasAttr() matcher instead).

I think the logic here should be:

if (const auto *CIA = Node.getAttr<ConstInitAttr>())
  return CIA->isConstinit();
return false;

and you should add a test case + documentation to show that we don't match the attribute form.

Izaron updated this revision to Diff 402111.Jan 21 2022, 2:40 PM

Fix: accept only keyword, not the attribute itself

Izaron marked an inline comment as done.Jan 21 2022, 2:46 PM
Izaron added inline comments.
clang/include/clang/ASTMatchers/ASTMatchers.h
5223

Thank you! Very valuable information. It opened my eyes why is constinit unexpectedly parsed like an attribute. Another plus of the new matcher - users won't search for constinit incorrectly as I did before the fix.

I added an explicit example to docs showing that the clang attribute won't work.

Izaron marked an inline comment as done.Jan 21 2022, 2:47 PM
This revision is now accepted and ready to land.Jan 24 2022, 5:34 AM
aaron.ballman closed this revision.Jan 24 2022, 5:37 AM

I've commit on your behalf in 589a93907222cf2380198ca2172ff6697dd43d87, thanks for the new matcher!