gchatelet marked 3 inline comments as done.
gchatelet added inline comments.
================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:1098-1099
+
+ if (D->hasAttr<NoBuiltinAttr>())
+ D->dropAttr<NoBuiltinAttr>();
+
----------------
gchatelet wrote:
> aaron.ballman wrote:
> > Just making sure I understand the semantics you want: redeclarations do not
> > have to have matching lists of builtins, but instead the definition will
> > use a merged list? e.g.,
> > ```
> > [[clang::no_builtin("memset")]] void whatever();
> > [[clang::no_builtin("memcpy")]] void whatever();
> >
> > [[clang::no_builtin("memmove")]] void whatever() {
> > // Will not use memset, memcpy, or memmove builtins.
> > }
> > ```
> > That seems a bit strange, to me. In fact, being able to apply this
> > attribute to a declaration seems a bit like a mistake -- this only impacts
> > the definition of the function, and I can't imagine good things coming from
> > hypothetical code like:
> > ```
> > [[clang::no_builtin("memset")]] void whatever();
> > #include "whatever.h" // Provides a library declaration of whatever() with
> > no restrictions on it
> > ```
> > WDYT about restricting this attribute to only appear on definitions?
> That's a very good point. Thx for noticing.
> Indeed I think it only makes sense to have the attribute on the function
> definition.
>
> I've tried to to use `FunctionDecl->hasBody()` during attribute handling in
> the Sema phase but it seems like the `FunctionDecl` is not complete at this
> point.
> All calls to `hasBody()` return `false`, if I repeat the operation in
> `CGCall` then `hasBody` returns `true` and I can see the `CompoundStatement`.
>
> Do you have any recommendations on where to perform the check?
So after some investigations it turns out that
`FunctionDecl::isThisDeclarationADefinition` is buggy (returns always `false`)
when called from `ProcessDeclAttribute`.
I believe it's because the `CompoundStatement` is not parsed at this point.
As a matter of fact all code using this function in `ProcessDeclAttribute` is
dead code (see D68379 which disables the dead code, tests are still passing)
I'm unsure of how to fix this, it may be possible of using
`FunctionDecl::setWillHaveBody`in [[
https://github.com/llvm/llvm-project/blob/0577a0cedbc5be4cd4c20ba53d3dbdac6bff9a0a/clang/lib/Sema/SemaDecl.cpp#L8820
| this switch ]].
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D68028/new/
https://reviews.llvm.org/D68028
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
