[PATCH] D96109: Refactor implementation of -funique-internal-linkage-names.

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
Comment at: clang/lib/AST/Mangle.cpp:119-124
+  // C functions with internal linkage have to be mangled with option
+  // -funique-internal-linkage-names.
+  if (!getASTContext().getLangOpts().CPlusPlus &&
+      isUniqueInternalLinkageDecl(D))
+    return true;
+
----------------
Strikes me as strange to test the language mode here (presumably this logic would apply equally to C++ code, right? If at most it'd be a no-op because things are already mangled)

But, for instance - what about functions with extern "C" linkage in a C++ file?


================
Comment at: clang/lib/CodeGen/CodeGenModule.cpp:205
+    // that this symbol is of internal linkage type.
+    ModuleNameHash = (Twine(".__uniq.")
+                      + Twine(IntHash.toString(10, false))).str();
----------------
Does this need the double underscore? I thought other suffixes here generally don't have that feature?


================
Comment at: clang/lib/CodeGen/CodeGenModule.cpp:206
+    ModuleNameHash = (Twine(".__uniq.")
+                      + Twine(IntHash.toString(10, false))).str();
+  }
----------------
Might be worth a parameter name comment explaining what the 'false' parameter means? (I guess the 10 means the numeric base to use)


================
Comment at: clang/lib/CodeGen/CodeGenModule.cpp:1179-1182
+      ((isa<FunctionDecl>(D) &&
+        CGM.getFunctionLinkage(GD) == llvm::GlobalValue::InternalLinkage) ||
+       (isa<VarDecl>(D) && CGM.getContext().GetGVALinkageForVariable(
+                               cast<VarDecl>(D)) == GVA_Internal)))
----------------
The variety of things being tested here seems non-trivial, and I've no idea how to validate it's correct. Any chance this is existing logic that could be reused from somewhere else/refactored into a common function to call from some existing use case and this new one? (similar thougts on ItaniumDemangle.cpp:isInternalLinkageDecl)


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D96109/new/

https://reviews.llvm.org/D96109