[cfe-commits] Greatly improved static_cast error messages

[Sebastian Redl]

Douglas Gregor wrote:
>
> 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!
Thank you!
>
> 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?
Correct.
>
> 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.
OK.
>
> 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."
Yes, but then the later conversion also needs a flag that says, "an 
earlier check may have had a better reason to complain than you, so 
don't", and a return value that says, "I would have complained, but you 
told me not to".
Or perhaps that can actually be solved nicely. I'll add the fixme and 
reinvestigate at a later date.
>
> In TryStaticImplicitCast:
>
> +  if (DestType->isRecordType()) {
> +    // FIXME: Use an implementation of C++ [over.match.ctor] for this.
> +    return TSC_NotApplicable;
> +  }
>
> Have you seen PerformInitializationByConstructor?
Yes, but I haven't had the time to examine it more closely yet. Since 
the old version didn't handle this case, I didn't add the case now either.
> 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.
Probably.
>
> +  // 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.
Yep, that's it.

Thanks for the review.