[cfe-commits] [patch][pr13142] Call the correct method when looking through casts to final classes.

[Douglas Gregor]

On Jun 20, 2012, at 12:48 PM, Rafael Espíndola wrote:

>> ... I could not
>> find a good place is sema to mark them used. The regular functions in
>> the previous examples are not, but we were asserting only on
>> destructors.
> 
> This patch is a variation that moves code into the AST library so that
> Sema can mark the destructor used. It is not pretty, as now there is
> some duplication of effort in Sema and Codegen, but the Codegen assert
> about the constructor being consider used by Sema was useful in the
> past, so it would be a pity to drop it.
> 
> Let me know which one you like best.


The codegen assertion is extremely important, because it makes sure that Sema will have instantiated all of the templates that codegen needs, so this version is the way to go. Some comments:

index 6fe71c8..88d5573 100644
--- a/include/clang/AST/DeclCXX.h
+++ b/include/clang/AST/DeclCXX.h
@@ -1630,7 +1630,15 @@ public:
   /// supplied by IR generation to either forward to the function call operator
   /// or clone the function call operator.
   bool isLambdaStaticInvoker() const;
-  
+
+  CXXMethodDecl *
+  getCorrespondingMethodInClass(const CXXRecordDecl *RD);
+
+  const CXXMethodDecl *
+  getCorrespondingMethodInClass(const CXXRecordDecl *RD) const {
+    return const_cast<CXXMethodDecl*>(this)->getCorrespondingMethodInClass(RD);
+  }
+

These need documentation.

diff --git a/include/clang/AST/Expr.h b/include/clang/AST/Expr.h
index 8c3712d..0d06fb3 100644
--- a/include/clang/AST/Expr.h
+++ b/include/clang/AST/Expr.h
@@ -665,6 +665,8 @@ public:
 
   static bool hasAnyTypeDependentArguments(llvm::ArrayRef<Expr *> Exprs);
 
+  const CXXRecordDecl *getMostDerivedClassDecl() const;
+
   static bool classof(const Stmt *T) {
     return T->getStmtClass() >= firstExprConstant &&
            T->getStmtClass() <= lastExprConstant;

This needs documentation and a more descriptive name, because an expression doesn't naturally have a 'most derived class decl'. 

+static bool recursivellyOverrides(const CXXMethodDecl *DerivedMD,
+                                  const CXXMethodDecl *BaseMD) {
+  for (CXXMethodDecl::method_iterator I = DerivedMD->begin_overridden_methods(),
+         E = DerivedMD->end_overridden_methods(); I != E; ++I) {
+    const CXXMethodDecl *MD = *I;
+    if (MD == BaseMD)
+      return true;
+    if (recursivellyOverrides(MD, BaseMD))
+      return true;
+  }
+  return false;
+}

You should compare MD->getCanonicalDecl() == BaseMD->getCanonicalDecl() here.

+CXXMethodDecl *
+CXXMethodDecl::getCorrespondingMethodInClass(const CXXRecordDecl *RD) {
+  // FIXME: Is there an easier way of iterating over all methods (inherited
+  // or not)?
+  // Should this be using some walker?
+
+  if (this->getParent() == RD)
+    return this;

I think you also want to compare canonical declarations here or, at the very least, get the definition of RD.

+  for (CXXRecordDecl::method_iterator I = RD->method_begin(),
+         E = RD->method_end(); I != E; ++I) {
+    CXXMethodDecl *DMD = *I;
+    if (recursivellyOverrides(DMD, this))
+      return DMD;
+  }

This is doing a lot of excess work, because you're walking the overrides of completely unrelated methods. At the very least, you should lookup() on the name of the method and only walk those. A 'perfect' solution would use the rules that govern the signatures of overridden functions (e.g., by checking parameter types and cv/ref-qualifiers) to avoid looking at functions that clearly can't override 'this'.

+  for (CXXRecordDecl::base_class_const_iterator I = RD->bases_begin(),
+         E = RD->bases_end(); I != E; ++I) {
+    const CXXRecordDecl *Base =
+      cast<CXXRecordDecl>(I->getType()->getAs<RecordType>()->getDecl());
+    CXXMethodDecl *T = this->getCorrespondingMethodInClass(Base);
+    if (T)
+      return T;
+  }
+
+  return NULL;
+}

We should be a bit more careful with the result of getAs<RecordType>() here; this routine is going to explode if called on a class template or member thereof, if it has a dependent base. It's best to be robust against such things in the AST library.

+const CXXRecordDecl *Expr::getMostDerivedClassDecl() const {
+  const Expr *E = this;
+
+  while (true) {
+    E = E->IgnoreParens();
+    if (const CastExpr *CE = dyn_cast<CastExpr>(E)) {
+      if (CE->getCastKind() == CK_DerivedToBase ||
+          CE->getCastKind() == CK_UncheckedDerivedToBase ||
+          CE->getCastKind() == CK_NoOp) {
+        E = CE->getSubExpr();
+        continue;
+      }
+    }
+
+    break;
+  }
+
+  QualType DerivedType = E->getType();
+  if (DerivedType->isDependentType())
+    return NULL;
+  if (const PointerType *PTy = DerivedType->getAs<PointerType>())
+    DerivedType = PTy->getPointeeType();
+
+  const RecordType *Ty = DerivedType->castAs<RecordType>();
+  Decl *D = Ty->getDecl();
+  return cast<CXXRecordDecl>(D);
+}

This needs to either assert() that we actually have something of (pointer-to-)class type, or it has to be robust against the possibility of a non-

	- Doug