Re: [PATCH] D11297: PR17829: Functions declared extern "C" with a name matching a mangled C++ function are allowed

2015-08-31 Thread Andrey Bokhanko via cfe-commits
andreybokhanko added a comment. In http://reviews.llvm.org/D11297#235525, @rjmccall wrote: > Yes, please make it an error. Done. John, thank you for all your patience and explanations! -- I understand that this particular review and patch author required more than the usual measure. :-( >

Re: [PATCH] D11297: PR17829: Functions declared extern "C" with a name matching a mangled C++ function are allowed

2015-08-31 Thread Phabricator via cfe-commits
This revision was automatically updated to reflect the committed changes. Closed by commit rL246438: PR17829: Proper diagnostic of mangled names conflicts (authored by asbokhan). Changed prior to commit: http://reviews.llvm.org/D11297?vs=33401=33577#toc Repository: rL LLVM

Re: [PATCH] D11297: PR17829: Functions declared extern C with a name matching a mangled C++ function are allowed

2015-08-28 Thread Andrey Bokhanko via cfe-commits
andreybokhanko added inline comments. Comment at: include/clang/Basic/DiagnosticSemaKinds.td:2323 @@ -2323,1 +2322,3 @@ + definition with same mangled name as another definition, + InGroupDuplicateMangledNames; def err_cyclic_alias : Error rjmccall wrote: I'm

Re: [PATCH] D11297: PR17829: Functions declared extern C with a name matching a mangled C++ function are allowed

2015-08-28 Thread John McCall via cfe-commits
rjmccall added a comment. Yes, please make it an error. And the obvious test changes are fine. :) http://reviews.llvm.org/D11297 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D11297: PR17829: Functions declared extern C with a name matching a mangled C++ function are allowed

2015-08-27 Thread Andrey Bokhanko via cfe-commits
andreybokhanko marked 3 inline comments as done. andreybokhanko added a comment. John, Thank you for the review! All your comments but one are fixed. See below for details on the single one I didn't manage to get fixed. Andrey Comment at: lib/CodeGen/CodeGenModule.h:354 @@

Re: [PATCH] D11297: PR17829: Functions declared extern C with a name matching a mangled C++ function are allowed

2015-08-27 Thread John McCall via cfe-commits
rjmccall added inline comments. Comment at: lib/CodeGen/CodeGenModule.h:354 @@ +353,3 @@ + /// call). + llvm::DenseSetGlobalDecl ExplicitDefinitions; + andreybokhanko wrote: Checking that a GlobalDecl is not in ExplicitDefinitions yet is actually required to

Re: [PATCH] D11297: PR17829: Functions declared extern C with a name matching a mangled C++ function are allowed

2015-08-25 Thread Andrey Bokhanko via cfe-commits
andreybokhanko updated this revision to Diff 33083. andreybokhanko added a comment. John, I implemented precisely what you described (or so I believe :-)) Patch is updated; please re-review. This patch implements support for functions, but not variables yet -- the patch is big enough already,

Re: [PATCH] D11297: PR17829: Functions declared extern C with a name matching a mangled C++ function are allowed

2015-08-25 Thread John McCall via cfe-commits
rjmccall added a comment. This looks generally like what I'm looking for, thanks! Some comments. Comment at: lib/CodeGen/CodeGenModule.cpp:1129 @@ +1128,3 @@ +if (GV GV != GetGlobalValue(getMangledName(D))) + continue; + This is a pretty expensive

Re: [PATCH] D11297: PR17829: Functions declared extern C with a name matching a mangled C++ function are allowed

2015-08-13 Thread Andrey Bokhanko via cfe-commits
andreybokhanko added a comment. John, Thank you for the quick reply! Let me make sure I understand what you said, using my test as an example (BTW, sorry if this is a dumb question -- I asked our local Clang experts, but no-one seems to be 100% sure what to do): 1: struct T { 2: ~T()

Re: [PATCH] D11297: PR17829: Functions declared extern C with a name matching a mangled C++ function are allowed

2015-08-13 Thread John McCall via cfe-commits
rjmccall added a comment. In http://reviews.llvm.org/D11297#223622, @andreybokhanko wrote: John, Thank you for the quick reply! Let me make sure I understand what you said, using my test as an example (BTW, sorry if this is a dumb question -- I asked our local Clang experts, but no-one

Re: [PATCH] D11297: PR17829: Functions declared extern C with a name matching a mangled C++ function are allowed

2015-08-12 Thread John McCall via cfe-commits
rjmccall added a comment. You only have one attempt to define the function here; I don't see the problem. Recall that I said to add a flag to getOrCreateLLVMFunction that says whether the caller intends to define the function. The rule should be that only callers that pass true should be