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

[Sebastian Pop via cfe-commits]

sebpop added inline comments.

================
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.
----------------
rsmith wrote:
> It's not clear what this comment about `AlwaysInline` is referring to, since the code does not mention that.
I think we should just remove all the FIXME note: Aditya has just moved this comment from below... to here.

================
Comment at: clang/lib/CodeGen/CGCXX.cpp:162
@@ -161,3 @@
-      !TargetDecl.getDecl()->hasAttr<AlwaysInlineAttr>())) {
-    // FIXME: An extern template instantiation will create functions with
-    // linkage "AvailableExternally". In libc++, some classes also define
----------------
... from here ...

================
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);
----------------
rsmith wrote:
> 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?
I think this change does not modify the current behavior: the check has been moved up and we early return in that case.


https://reviews.llvm.org/D24682