aaron.ballman added inline comments.
================
Comment at: clang/lib/Sema/SemaDecl.cpp:9508-9510
+ // FIXME: We should really be doing this in SemaDeclAttr.cpp::handleNoBuiltin
+ // but there is a bug with FunctionDecl::isThisDeclarationADefinition() which
+ // always returns false before Sema::ActOnStartOfFunctionDef is called.
----------------
rsmith wrote:
> aaron.ballman wrote:
> > handleNoBuiltin -> handleNoBuiltinAttr
> I am not convinced that this is a bug -- the function declaration does not
> become a definition until the parser reaches the definition.
>
> In any case, I don't think the predicate you want is "is this declaration a
> definition?". Rather, I think what you want is, "does this declaration
> introduce an explicit function body?". In particular, we should not permit
> uses of this attribute on defaulted or deleted functions, nor on functions
> that have a definition by virtue of using `__attribute__((alias))`. So it
> probably should be a syntactic check on the form of the declarator (that is,
> the check you're perrforming here), and the check should probably be
> `D.getFunctionDefinitionKind() == FDK_Definition`. (A custom diagnostic for
> using the attribute on a defaulted or deleted function would be nice too,
> since the existing diagnostic text isn't really accurate in those cases.)
> In particular, we should not permit uses of this attribute on defaulted or
> deleted functions
Deleted functions, sure. Defaulted functions... not so sure. I could sort of
imagine wanting to instruct a defaulted assignment operator that does
memberwise copy that it's not allowed to use a builtin memcpy, for instance. Or
is this a bad idea for some reason I'm not thinking of?
================
Comment at: clang/test/Sema/no-builtin.c:7
+
+void no_builtin_no_argument() __attribute__((no_builtin)) {}
+// expected-error@-1 {{'no_builtin' attribute takes at least 1 argument}}
----------------
rsmith wrote:
> I find this surprising. I would expect this to work, and to mean the same as
> `-fno-builtin` for the function (that is, same as the `*` form).
>
> I think might be a better design to remove the magical `"*"` form entirely,
> and use an empty argument list to mean "all builtins". Have you considered
> that? (That would better mirror how `-fno-builtin` vs `-fno-builtin-foo`
> works.)
> I think might be a better design to remove the magical "*" form entirely, and
> use an empty argument list to mean "all builtins".
I like this approach.
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
