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

[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 extra check, and I think it only kicks in on invalid code where we've got multiple definitions of a function.  Can we just eliminate it?  It's not really a problem to emit the second function definition as long as we're not trying to emit it into the same llvm::Function.

================
Comment at: lib/CodeGen/CodeGenModule.cpp:1573
@@ +1572,3 @@
+static void replaceUsesOfNonProtoConstant(llvm::Constant *old,
+                                          llvm::Function *newFn) {
+  // Fast path.
----------------
Instead of moving this function in this patch, just add a forward declaration.  If you want to move it, you can do that in a separate patch that only moves the function.

================
Comment at: lib/CodeGen/CodeGenModule.h:349
@@ +348,3 @@
+  llvm::SmallVector<std::pair<llvm::GlobalValue *, llvm::Constant *>, 8>
+    GlobalValReplacements;
+
----------------
Missing a period in the comment.

================
Comment at: lib/CodeGen/CodeGenModule.h:354
@@ +353,3 @@
+  /// call).
+  llvm::DenseSet<GlobalDecl> ExplicitDefinitions;
+
----------------
I don't think this is necessary.  I don't believe we ever need to emit a (mangled) function for definition between starting to emit another function and adding its entry block, or between starting to emit a global variable and finishing its constant initializer.  So you should be able to just check whether the existing llvm::GlobalValue is a definition.

I don't think avoiding emitting the diagnostic multiple times for different globals is necessary, or even a good idea.


http://reviews.llvm.org/D11297