[PATCH] D56366: New warning call-to-virtual-from-ctor-dtor when calling a virtual function from a constructor or a destructor

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.


================
Comment at: lib/Sema/SemaOverload.cpp:13075
+  if ((isa<CXXConstructorDecl>(CurContext) ||
+       isa<CXXDestructorDecl>(CurContext)) &&
+      isa<CXXThisExpr>(MemExpr->getBase()->IgnoreParenCasts()) &&
----------------
Intentionally suppressed in lambdas?  I think that might be reasonable, but also worth a comment.


================
Comment at: lib/Sema/SemaOverload.cpp:13080
+      // We already have a warning for pure virtual functions above, so don't
+      // consider them here
+      !TheCall->getMethodDecl()->isPure()) {
----------------
This should be folded into the same checks that lead into that warning, since most of the conditions are identical and it's just a few final decisions (and the purity of the function) that changes whether to emit a diagnostic and which one.


================
Comment at: lib/Sema/SemaOverload.cpp:13087
+    const CXXMethodDecl *ContextMD = dyn_cast<CXXMethodDecl>(CurContext);
+    assert(ContextMD && "CurContext was expected be a method declaration");
+    const CXXRecordDecl *ContextRD = ContextMD->getParent();
----------------
Use `cast<>` instead of `dyn_cast`+`assert`.


================
Comment at: lib/Sema/SemaOverload.cpp:13091
+    // final
+    if (ContextRD->isPolymorphic() && !ContextRD->hasAttr<FinalAttr>()) {
+      Diag(MemExpr->getBeginLoc(),
----------------
`ContextRD` must be polymorphic, it provides a `virtual` method.


================
Comment at: lib/Sema/SemaOverload.cpp:13099
+          << FixItHint::CreateInsertion(
+                 MemExpr->getBeginLoc(),
+                 ContextMD->getParent()->getDeclName().getAsString() + "::");
----------------
As a matter of general principle, these diagnostics should be at `TheCall->getExprLoc()` instead of `MemExpr->getBeginLoc()`.  As a matter of correctness, the insertion must start at `MemExpr->getMemberLoc()` instead of `MemExpr->getBeginLoc()` because the base in the member expression might be explicit.

Please add tests for the fix-its.


Repository:
  rC Clang

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

https://reviews.llvm.org/D56366