[PATCH] D40819: Implement Attribute Target MultiVersioning (Improved edition!)

[Erich Keane via Phabricator via cfe-commits]

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