erichkeane marked 17 inline comments as done.
erichkeane added a comment.
Patch incoming, Thank you very much for the review @aaron.ballman
================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:9333-9335
+ "multiversion function would have identical mangling to a previous "
+ "definition. Duplicate declarations must have identical target attribute "
+ "values">;
----------------
aaron.ballman wrote:
> Diagnostics are not complete sentences, so this should be reworded to be even
> less grammatically correct. ;-)
I tried again, hopefully this one is better? :) Let me know if my grammar is
still too good...
================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:9339-9340
+def err_multiversion_no_other_attrs : Error<
+ "attribute 'target' multiversioning cannot be combined with other "
+ "attributes">;
+def err_multiversion_diff : Error<
----------------
aaron.ballman wrote:
> This worries me slightly. Is there a benefit to prohibiting this attribute
> with any other attribute? For instance, I'm thinking about a multiversioned
> noreturn function or a multiversioned function with a calling convention as
> plausible use cases.
This attribute can have negative effects if combined with certain others,
though I don't have a complete list. GCC seems to 'ignore' a handful of
others, but I don't have the complete list. It is my intent to disallow all
others in this patch, and upon request/further work, permit them as an opt-in.
================
Comment at: lib/Sema/SemaDecl.cpp:9448
+ if (NewFD->isMain()) {
+ if (NewTA && NewTA->getFeaturesStr() == "default") {
+ S.Diag(NewFD->getLocation(), diag::err_multiversion_not_allowed_on_main);
----------------
aaron.ballman wrote:
> So any other form of target attribute and feature string is fine to place on
> `main()`?
It IS, it just doesn't cause multiversioning. The "return false" at the bottom
of this condition returns to CheckFunctionDeclaration.
https://reviews.llvm.org/D40819
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
