[PATCH] D24682: [PR30341] Alias must point to a definition

[Richard Smith via cfe-commits]

rsmith added inline comments.

================
Comment at: clang/lib/CodeGen/CGCXX.cpp:137-138
@@ -136,1 +136,4 @@
 
+  // Disallow aliases to available_externally because available_externally
+  // will not be there in the end to allow the creation of the alias (PR30341).
+  // FIXME: An extern template instantiation will create functions with
----------------
Instead of this, I'd just say:

> available_externally definitions aren't real definitions, so we cannot create an alias to one.

or similar. It doesn't seem necessary or relevant to reference a particular PR here.

================
Comment at: clang/lib/CodeGen/CGCXX.cpp:140-142
@@ +139,5 @@
+  // FIXME: An extern template instantiation will create functions with
+  // linkage "AvailableExternally". In libc++, some classes also define
+  // members with attribute "AlwaysInline" and expect no reference to
+  // be generated. It is desirable to reenable this optimisation after
+  // corresponding LLVM changes.
----------------
It's not clear what this comment about `AlwaysInline` is referring to, since the code does not mention that.

================
Comment at: clang/lib/CodeGen/CGCXX.cpp:142-143
@@ +141,4 @@
+  // members with attribute "AlwaysInline" and expect no reference to
+  // be generated. It is desirable to reenable this optimisation after
+  // corresponding LLVM changes.
+  if (TargetLinkage == llvm::GlobalValue::AvailableExternallyLinkage)
----------------
What "corresponding LLVM changes" are you expecting here? It seems to be fundamental to aliases that they can only denote definitions.

================
Comment at: clang/lib/CodeGen/CGCXX.cpp:170
@@ -159,9 +169,3 @@
   if (llvm::GlobalValue::isDiscardableIfUnused(Linkage) &&
-     (TargetLinkage != llvm::GlobalValue::AvailableExternallyLinkage ||
-      !TargetDecl.getDecl()->hasAttr<AlwaysInlineAttr>())) {
-    // FIXME: An extern template instantiation will create functions with
-    // linkage "AvailableExternally". In libc++, some classes also define
-    // members with attribute "AlwaysInline" and expect no reference to
-    // be generated. It is desirable to reenable this optimisation after
-    // corresponding LLVM changes.
+      !TargetDecl.getDecl()->hasAttr<AlwaysInlineAttr>()) {
     addReplacement(MangledName, Aliasee);
----------------
Did you mean to change the behavior here? For non-`available_externally` functions, we never used to care whether they're `always_inline`. Why do we care now?


https://reviews.llvm.org/D24682