Izaron marked an inline comment as done.
Izaron added inline comments.
================
Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:5223
+/// matches the declaration of foo and bar.
+AST_MATCHER(VarDecl, isConstinit) { return Node.hasAttr<ConstInitAttr>(); }
+
----------------
aaron.ballman wrote:
> 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.
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.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D117846/new/
https://reviews.llvm.org/D117846
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
