Hello John, Thanks for review!
I attached a quiet version of a patch that doesn't warn if any of the paths is a non-virtually-derived base at offset zero. During first iteration, Jordan proposed to make a separate switch to enable a noisy version that warns even if the cast is safe, though. If it is still required, it can be easily added back. Processing multiple base paths made checking a bit heavier, but I see no other approach. On 12 March 2013 20:14, John McCall <[email protected]> wrote: > On Mar 12, 2013, at 10:01 AM, Jordan Rose <[email protected]> wrote: > > Pinging this myself, adding Richard Smith as another reviewer. I don't > quite feel qualified to give the final word on it, but I'm fixing an > analyzer bug that's basically "don't crash when the user does this in > exactly the right wrong way", and it'd be nice to have a warning so that > they fix their code. > > +def ReinterpretUpDownCastZeroAdjustment : > + DiagGroup<"reinterpret-updown-zero-adjustment">; > +def ReinterpretUpDownCast : DiagGroup<"reinterpret-updown", > > How about -Wreinterpret-base and -Wreinterpret-primary-base? We only need reinterpret-primary-base now. +def warn_reinterpret_wrong_subobject : Warning< > + "'reinterpret_cast' might return a pointer to wrong subobject">, > > How about "reinterpret_cast %select{from|to}3 class %0 %select{to|from}3 > %select{virtual base|base at non-zero offset}2 %1 behaves differently from > static_cast"? > Fixed. Shouldn't we provide some details on what causes the difference? > +static void DiagnoseReinterpretUpDownCast(Sema &Self, ExprResult &SrcExpr, > + QualType DestType, > + const SourceRange &OpRange) { > > You should take SrcExpr as a const Expr * and OpRange as a SourceRange. > Fixed. > > + const Type *SrcUnqualType = SrcType.getTypePtr(); > + const Type *DestUnqualType = DestType.getTypePtr(); > > This is unnecessary and also doesn't necessarily actually give you an > unqualified type. > Deleted. > > + // When casting from pointer or reference, get pointee type; use > original > + // type otherwise. > + const CXXRecordDecl *SrcPointeeRD = > SrcUnqualType->getPointeeCXXRecordDecl(); > + const CXXRecordDecl *SrcRD = > + SrcPointeeRD ? SrcPointeeRD : SrcUnqualType->getAsCXXRecordDecl(); > > It'd be good to remember the original non-pointer sugared types here for > diagnostic purposes. > > Also, at this point in semantic analysis you can differentiate between the > pointer and reference cases by just checking SrcExpr->isRValue(). Warning "wrong cast to T*" looks clearer to me than "wrong cast to T" if the code is reinterpret_cast<T *>(...). > > + CXXBasePaths SrcBP, DestBP; > + > ... > + const ASTRecordLayout &SrcLayout = > Self.Context.getASTRecordLayout(SrcRD); > + const ASTRecordLayout &DestLayout = > Self.Context.getASTRecordLayout(DestRD); > > 1. Please sink this down to the point where you need it. > 2. Please only actually get the layout for the derived class. We actually need the layout of any derived class in the pass to process indirect base offsets correctly. > 3. You can re-use the paths object if the first lookup failed. > + if (SrcBP.getDetectedVirtual() || DestBP.getDetectedVirtual()) { > > You are ignoring the possibility that there are subobjects at multiple > paths. We should not warn if *any* of the paths is a non-virtually-derived > base at offset zero. > + CharUnits Offset = CharUnits::Zero(); > + if (ReinterpretKind == ReinterpretUpcast) > + Offset = SrcLayout.getBaseClassOffset(DestRD); > + else if (ReinterpretKind == ReinterpretDowncast) > + Offset = DestLayout.getBaseClassOffset(SrcRD); > > This won't work for indirect base classes. > + DiagnoseReinterpretUpDownCast(Self, SrcExpr, DestType, OpRange); > + > > Please sink this until we've decided that the cast succeeds. Fixed all of the above. > > John. > > -- Alex
13824quiet.patch
Description: Binary data
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
