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

[Erich Keane via Phabricator via cfe-commits]

erichkeane marked 7 inline comments as done.
erichkeane added a comment.

patch incoming!



================
Comment at: include/clang/AST/ASTContext.h:2643-2648
+    for (auto *CurDecl :
+         FD->getDeclContext()->getRedeclContext()->lookup(FD->getDeclName())) {
+      FunctionDecl *CurFD = CurDecl->getAsFunction()->getCanonicalDecl();
+      if (CurFD && hasSameType(CurFD->getType(), FD->getType()))
+        P(CurFD);
+    }
----------------
rsmith wrote:
> This should deal with the case where `lookup` finds multiple declarations of the same version (which can happen in particular when serializing to AST files; we keep around a handle to the version from each AST file). Also, consider moving this to the .cpp file (use `llvm::function_ref<void(const FunctionDecl*)>` to pass in the callback).
Ah, right, this part got dropped when I was moving it around...  

There is only 2 ways I could think of to do this properly, first is to actually check the 'target' strings, second is to check the address of the canonical decl.  Checking target strings seems expensive, but I don't know if the pointer values are sufficient?

I'll implement the 2nd for this patch, but if you could give me a hint as the proper way (if this isn't sufficient), I'd be grateful.


================
Comment at: lib/Sema/SemaDecl.cpp:9228-9229
+    S.Diag(NewFD->getLocation(), diag::err_multiversion_no_other_attrs);
+    if (CausesMV)
+      S.Diag(NewFD->getLocation(), diag::note_multiversioning_caused_here);
+    return true;
----------------
rsmith wrote:
> It seems strange to have a "caused here" note pointing at the same location that you just produced an error on. Is this note really adding value? (Likewise on all later diagnostics that add this note at the same place as the error.)
I think that makes sense.  I'll remove all of those diagnostics where the error and the 'caused by' are different lines.


https://reviews.llvm.org/D40819