On Mar 14, 2013, at 8:47 AM, Alexander Zinenko <[email protected]> wrote: > 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.
+ if(!SrcRD || !SrcRD->hasDefinition()) + return; + + const CXXRecordDecl *DestRD = DestType->getPointeeCXXRecordDecl(); + + if(!DestRD || !DestRD->hasDefinition()) + return; Please add a comment saying that we're deliberately not instantiating templates here because we're not allowed to. Also, I think this needs to use isCompleteDefinition(). > 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. Let's call it -Wreinterpret-base-class, actually. > +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? Well, my suggestion mentions that it's a virtual base or a base at a non-zero offset, but if you want something more or different, feel free. > +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 *>(...). That's fine. > > + 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. Good point. But we definitely don't need the layout of the base class. :) + enum { + ReinterpretNormalCast = -1, + ReinterpretUpcast, + ReinterpretDowncast + } ReinterpretKind; + ReinterpretKind = ReinterpretNormalCast; You don't need ReinterpretNormalCast. Just leave it uninitialized; any reasonable -Wuninitialized will be able to figure out that it's initialized along every path. Otherwise, this looks great. Feel free to check in with these changes. John.
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
