[PATCH] D34301: [Sema] Make sure the definition of a referenced virtual function is emitted when it is final

[Akira Hatanaka via Phabricator via cfe-commits]

ahatanak marked 2 inline comments as done.
ahatanak added inline comments.


================
Comment at: lib/Sema/SemaExpr.cpp:14715
+    if (Method->isVirtual() && !(Method->hasAttr<FinalAttr>() ||
+                                 Method->getParent()->hasAttr<FinalAttr>()))
       OdrUse = false;
----------------
vsk wrote:
> Do you think it makes sense to eliminate all candidate virtual methods which can be devirtualized? If so, you could make "CanDevirtualizeMemberFunctionCall" a shared utility between Sema and CodeGen, and use it here. That function should give "the truth" about whether or not a call can be devirtualized.
I moved CanDevirtualizeMemberFunctionCall to DeclCXX.cpp and added overloaded functions of canDevirtualizeCall and used one of them here. This caused changes in the generated IR in two test cases (no-devirt.cpp and vtable-available-externally.cpp). Both changes look correct to me.


================
Comment at: test/CodeGenCXX/devirtualize-virtual-function-calls-final.cpp:271
+        // Derived::operator() is not emitted, there will be a linker error.
+        (*ptr)();
+      }
----------------
vsk wrote:
> Have you looked into why "ptr->operator()();" compiles? We are either missing a devirtualization opportunity, or we have inconsistent logic for setting MightBeOdrUse for member calls. Either way, I think this patch is the right vehicle to address the issue.
"ptr->operator()();"  creates a MemberExpr, which MarkMemberReferenced handles. The difference between MarkMemberReferenced and MarkDeclRefReferenced is that the former sets OdrUse to false when the method is pure while the latter does so when the method is virtual.  Prior to r174341, MarkDeclRefReferenced was setting OdrUse to false for pure methods too (see r174242).


https://reviews.llvm.org/D34301