[clang] Bug 17456 cast diagnostic

[Richard Smith]

On Fri, Jan 23, 2015 at 12:37 PM, Nathan Sidwell <nathan at acm.org> wrote:
> Richard,
> I think this is the remaining patch of mine waiting for review.
>
> The original post is:
> http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20141201/119455.html
> but I duplicate the description here.  This updated patch fixes formatting,
> tabs and tweaks the error text, but otherwise is the same as the earlier
> posting.
>
> The patch addresses 17456, which is a desire for failed casts involving
> classes to give better diagnostic. see
> http://llvm.org/bugs/show_bug.cgi?id=17456

Thanks! This seems like a nice improvement to the diagnostics.

> Rather than a plain 'cannot cast' error, this patch changes things to:
>
> 1) for static cast from 'A *' to 'B *'  where T & S are classes, give
> 'static_cast from 'A *' to unrelated hierarchy 'B *' is not allowed'
>
> 2) where a cast fails and involves incomplete classes (directly or as a
> pointer), it also notes that the class is incomplete.  For instance:
>
> 17456.cc:6:10: error: static_cast from 'A *' to unrelated hierarchy 'B *' is
> not allowed

How about "static_cast from 'A *' to type 'B *' that is not related by
inheritance is not allowed"? As it stands, it sounds like you're
calling the type 'B *' a hierarchy. Also, consider:

  struct A {};
  struct B {};
  struct C : A, B {};

Here, A * and B * are not in unrelated hierarchies, so I'd prefer
something like "not related by inheritance" or "neither a base nor
derived class".

While here, please give err_bad_cxx_cast_unrelated a slightly more
specific name; [...]unrelated_class or similar?

>    return static_cast<B*>(arg);
>           ^~~~~~~~~~~~~~~~~~~~
> 17456.cc:3:8: note: 'B' is incomplete
> struct B;
>
>
> The test suite already had applicable cases,  so this patch just adjusts
> those
> tests.

+  // Detect if types are incomplete, and note it.
+  QualType Type = destType;
+  if (auto Ptr = Type->getAs<PointerType> ())
+    Type = Ptr->getPointeeType ();
+  if (auto Rec = Type->getAs<RecordType> ()) {
+    auto Decl = Rec->getAsCXXRecordDecl ();
+    if (!Decl->isCompleteDefinition ())
+      S.Diag (Decl->getLocation (), diag::note_type_incomplete)
+        << Decl->getDeclName ();
+  }
+
+  Type = src->getType();
+  if (auto Ptr = Type->getAs<PointerType> ())
+    Type = Ptr->getPointeeType ();
+  if (auto Rec = Type->getAs<RecordType> ()) {
+    auto Decl = Rec->getAsCXXRecordDecl ();
+    if (!Decl->isCompleteDefinition ())
+      S.Diag (Decl->getLocation (), diag::note_type_incomplete)
+        << Decl->getDeclName ();
+  }

I think we should only do this if we're casting to and from a class
type; I don't think it's useful to point out that T is an incomplete
type in:

  struct T *p;
  int *q = static_cast<int*>(p);

... since there is no way that completing the type of T would change
the outcome.

Also, we should produce a similar "unrelated hierarchy" diagnostic and
"incomplete" note when casting to a reference to incomplete class type
or casting from an lvalue of incomplete class type.

Finally, trivial formatting things:
 * remove the space before the parens in the function calls above.
(clang-format is a useful tool for consistently applying our preferred
formatting.)
 * put the && on the previous line when it's used in a line continuation:

+      if (SrcPointer->getPointeeType()->getAs<RecordType>()
+          && DestPointer->getPointeeType()->getAs<RecordType>())