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

[Vedant Kumar via Phabricator via cfe-commits]

vsk added a comment.

It looks



================
Comment at: lib/Sema/SemaExpr.cpp:14715
+    if (Method->isVirtual() && !(Method->hasAttr<FinalAttr>() ||
+                                 Method->getParent()->hasAttr<FinalAttr>()))
       OdrUse = false;
----------------
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.


================
Comment at: lib/Sema/SemaExpr.cpp:14717
       OdrUse = false;
   MarkExprReferenced(*this, E->getLocation(), E->getDecl(), E, OdrUse);
 }
----------------
"MarkExprReferenced" has what looks like an incomplete version of "CanDevirtualizeMemberFunctionCall". Do you think there is an opportunity to share logic there as well?


================
Comment at: test/CodeGenCXX/devirtualize-virtual-function-calls-final.cpp:271
+        // Derived::operator() is not emitted, there will be a linker error.
+        (*ptr)();
+      }
----------------
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.


https://reviews.llvm.org/D34301