[PATCH] D38596: Implement attribute target multiversioning

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: include/clang/Basic/Attr.td:1917
+      MVK_All, // All Decls of this function have a 'target' attribute.  None differ
+           // in contents, so this is the 'hint' case.
+      MVK_MultiVersion, // All Decls of this function have a 'target' attribute, some
----------------
Align the comments (at least the wrapped part of them)?


================
Comment at: lib/Sema/SemaDecl.cpp:9304
+
+static TargetAttr::MultiVersionKind getMultiVersionKind(FunctionDecl *OldFD) {
+  for (FunctionDecl *F : OldFD->redecls())
----------------
`OldFD` can be `const` (and that can be propagated to the other decls).


================
Comment at: lib/Sema/SemaDecl.cpp:9315
+
+  if (TA->getFeaturesStr() == "default")
+    return true;
----------------
Do you want to assert that `TA` is not null or handle treat null specially?


================
Comment at: lib/Sema/SemaDecl.cpp:9366
+  for (const auto *FD : OldFD->redecls())
+    Result = CheckMultiVersionOption(FD) && Result;
+
----------------
Reverse the conditions so the inexpensive bit is checked first, or early-break if `Result` becomes `false`.


================
Comment at: lib/Sema/SemaDecl.cpp:9368
+
+  return CheckMultiVersionOption(NewFD) && Result;
+}
----------------
Reverse conditions.


https://reviews.llvm.org/D38596