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



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to