[cfe-commits] Greatly improved static_cast error messages

[Douglas Gregor]

On Nov 6, 2008, at 3:53 PM, Sebastian Redl wrote:
> Attached patch brings a great improvement in the error messages for  
> invalid static_casts. It's not quite trivial, though, so I'm asking  
> for a review first.

These *are* greatly improved error messages. Nice!

Of course, you caught my eye with this one:

+  // This may not be entirely in line with the standard. Take for  
example:
+  // struct A {};
+  // struct B : virtual A {
+  //   B(A&);
+  // };
+  //
+  // void f()
+  // {
+  //   (void)static_cast<const B&>(*((A*)0));
+  // }
+  // As far as the standard is concerned, p5 does not apply (A is  
virtual), so
+  // p2 should be used instead - "const B& t(*((A*)0));" is perfectly  
valid.
+  // However, both GCC and Comeau reject this example, and there's no  
reason
+  // to go to absurdly great lengths just to support pathological  
examples.

I'm trying to determine what these "absurdly great lengths" actually  
would be. As far as I can tell, it means silently rejecting the A->B  
downcast and then trying conversion by constructor later on (which  
will happen anyway). That makes it harder to give a nice error message  
in the common case, right?

The reason for supporting this example, of course, is that it's what  
the standard says, and we can smugly add this to our list of things  
that we do right when others get them wrong. I'm okay with this patch  
going in as-is, despite being non-compliant in this area, but I'd  
rather that the comment reflect that this is a problem we're going to  
fix, but it isn't a very high priority, e.g., make it a FIXME and drop  
from "there's no reason..." to the end.

I guess TryStaticDowncast could have some kind of interface that  
allows it to say, "I might have more to complain about later" and, if  
that flag gets set and later conversions fail, we can go back through  
with a flag that says, "we failed; complain away."

In TryStaticImplicitCast:

+  if (DestType->isRecordType()) {
+    // FIXME: Use an implementation of C++ [over.match.ctor] for this.
+    return TSC_NotApplicable;
+  }

Have you seen PerformInitializationByConstructor? It's a relatively  
new addition that, as it says, performs initialization of a class type  
by constructor. It should do what you need, although I guess we might  
need to split it into Perform/TryInitializationByConstructor.

+  // FIXME: To get a proper error from invalid conversions here, we  
need to
+  // reimplement more of this.
+  ImplicitConversionSequence ICS = TryImplicitConversion(SrcExpr,  
DestType);
+  return ICS.ConversionKind ==  
ImplicitConversionSequence::BadConversion ?
+    TSC_NotApplicable : TSC_Success;

I assume that the problem here is that we don't get good error  
messages out of TryImplicitConversion? That's definitely true, and it  
has some of the same issues that you've been tackling with static_cast.

Thanks!

   - Doug