[cfe-dev] static_cast and dynamic_cast semantic checks
Sebastian Redl
sebastian.redl at getdesigned.at
Thu Oct 30 08:19:17 PDT 2008
Doug Gregor wrote:
> On Wed, Oct 29, 2008 at 4:39 PM, Sebastian Redl
> <sebastian.redl at getdesigned.at> wrote:
>
>> Another day, another update.
>>
>
> Thanks! I'll try not to keep stepping on your toes with these changes.
>
Don't worry about it. I don't see how you can avoid it anyway. And
pretty much every change you do ultimately improves my code as well.
> In Sema::CheckStaticCast:
>
> + // This option is unambiguous and simple, so put it here.
> + // C++ 5.2.9p4: Any expression can be explicitly converted to type "cv void".
> + if (DestType.getUnqualifiedType() == Context.VoidTy) {
> + return;
> + }
>
> You should use "DestType->isVoidType()" here, to catch typedefs of void.
>
OK.
> + // Reverse integral promotion/conversion. All such conversions are themselves
> + // again integral promotions or conversions and are thus already handled.
> + // (Note: any data loss warnings should be suppressed.)
> + // The exception is the reverse of enum->integer, i.e. integer->enum (and
> + // enum->enum). See also C++ 5.2.9p7.
> + // The same goes for reverse floating point promotion/conversion and
> + // floating-integral conversions. Again, only floating->enum is relevant.
> + if (DestType->isEnumeralType()) {
> + if ((SrcType->isRealType() && !SrcType->isVectorType()) ||
> + SrcType->isEnumeralType())
> + {
> + return;
> + }
> + }
>
> In the comment, it would help the reader if "already handled"
> mentioned where/how these conversions were handled. I realized
> (eventually) that it's a result of the implementation of paragraph 2,
> because the integral promotions/conversions, floating point
> promotions/conversions, and floating integral conversions are all
> reversible.
>
OK.
> isRealType threw me for a loop, since it's a C99 notion used in a C++
> context.
I basically went hunting among the various functions of Type to find one
which closely matched the simple concept I wanted - but those pesky
complex and vector types got in the way. :-)
> isArithmeticType matches the idea better (and the notion
> exists in both C99 and C++). I think I'd like the inner "if" to look
> like this:
>
> if (SrcType->isComplexType() || SrcType->isVectorType()) {
> // Fall though; no conversion from complex -> enum or vector -> enum.
> } else if (SrcType->isArithmeticType() || SrcType->isEnumeralType()) {
> return;
> }
>
Looks good.
> Moving on...
>
> + // Reverse pointer upcast. C++ 4.10p3 specifies pointer upcast.
> + // C++ 5.2.9p8 additionally disallows a cast path through virtual
> inheritance.
> + // (This is a practical limitation.)
> + if (IsStaticPointerDowncast(SrcType, DestType)) {
> + return;
> + }
>
> What does the comment (This is a practical limitation) mean? (Do we need it?)
>
It was a thought I had and which I put in a comment in the flow. We
don't need it.
As for what it means, it occurred to me at the time that a static cast
through a virtual base was technically impossible, since you don't know
the offset.
> + if (const PointerType *SrcPointer = SrcType->getAsPointerType()) {
> + QualType SrcPointee = SrcPointer->getPointeeType();
> + if (SrcPointee.getUnqualifiedType() == Context.VoidTy) {
>
> The last "if" should use the condition SrcPointee->isVoidType().
>
OK.
> In Sema::IsStaticDowncast:
>
> + BasePaths::paths_iterator Path = Paths.begin(), Path2 = Path;
> + ++Path2;
> + if (Path2 != Paths.end()) {
> + // If there is more than one path, but there's no ambiguity, there's got
> + // to be a virtual inheritance in the paths.
> + return false;
> + }
> +
> + for (BasePath::const_iterator PElem = Path->begin();
> + PElem != Path->end(); ++PElem)
> + {
> + if (PElem->Base->isVirtual()) {
> + return false;
> + }
> + }
>
> This code is checking whether SrcType is a virtual base of DestType.
> It definitely works as it is, but I think it could be both cleaner and
> more efficient if we moved the "is one of the paths virtual?" check
> into the BasePaths class. Then, Sema::IsDerivedFrom could just compute
> this flag as it's looking for the base class. Doing this,
> IsStaticDowncast would be able to set the BasePaths instance to avoid
> record paths, which will be more efficient. One could also imagine
> teaching Sema::IsDerivedFrom to abort earlier if it finds a path to a
> virtual base, using the same logic added to track whether there is a
> path through a virtual base.
>
Sounds great. Then I can also remember the virtual base and print it in
the error message, as GCC does.
> In Sema::CheckDynamicCast:
>
> + // Reference to void should be impossible.
> + assert(DestPointer);
>
> With this kind of thing, our scheme is to write the assert as:
>
> assert(DestPointer && "Reference to void is not possible");
>
> That way, the message spit out by assert() when it fails tells us what
> went wrong (roughly!).
>
OK. Strange, I usually do this anyway, but sometimes I don't, and I
never can remember why.
> + // C++ 5.2.7p5
> + // Upcasts are resolved statically.
> + if (DestRecord && IsDerivedFrom(SrcPointee, DestPointee)) {
> + CheckDerivedToBaseConversion(SrcPointee, DestPointee, OpLoc,
> SourceRange());
> + // Diagnostic already emitted on error.
> + return;
> + }
>
> Shouldn't the last argument to CheckDerivedToBaseConversion be the
> source range containing the full static_cast expression?
>
Could be. On the other hand, I never mark the entire cast expression on
conversion errors that involve both source and destination, so it would
be a bit inconsistent.
> bool Sema::IsIntegralPromotion(Expr *From, QualType FromType, QualType ToType)
> {
> const BuiltinType *To = ToType->getAsBuiltinType();
> + if (!To) {
> + return false;
> + }
>
> // An rvalue of type char, signed char, unsigned char, short int, or
> // unsigned short int can be converted to an rvalue of type int if
> // int can represent all the values of the source type; otherwise,
> // the source rvalue can be converted to an rvalue of type unsigned
> // int (C++ 4.5p1).
> - if (FromType->isPromotableIntegerType() &&
> !FromType->isBooleanType() && To) {
> + if (FromType->isPromotableIntegerType() && !FromType->isBooleanType()) {
>
> You only need the top change or the bottom change, not both; I suggest
> keeping the top one.
>
You seem to have this backward. There's a check for To not being null in
your existing code. Since my newly introduced check assures this, I
removed yours.
> @@ -590,8 +600,9 @@
>
> // An rvalue of type bool can be converted to an rvalue of type int,
> // with false becoming zero and true becoming one (C++ 4.5p4).
> - if (FromType->isBooleanType() && To && To->getKind() == BuiltinType::Int)
> + if (FromType->isBooleanType() && To->getKind() == BuiltinType::Int) {
> return true;
> + }
>
> return false;
> }
>
>
> We don't need this change, since the !To check is above.
>
As above.
> I'd like to see if the Sema::IsStaticDowncast/BasePaths can be
> optimized and cleaned up. Everything else is a minor comment, and
> things are in very good shape. Thanks!
>
OK, new patch to come soon. Thanks for the review.
Sebastian
More information about the cfe-dev
mailing list