Hello, John! Thanks for your comments and sorry that it's taking so long to polish my code. Please find the updated version attached.
On 20 March 2013 07:02, John McCall <[email protected]> wrote: > 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. > Actually, we do not call CheckReinterpretCast if the cast is type-dependent. The reason behind these checks is the following case with forward declaration class A; class B {}; void foo(A *a) { reinterpret_cast<B *>(a); } in which we can't examine the definition of A since we don't have it yet. I added a comment and mentioned templates at the end though. -- Alex > > 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. >
13824quiet.patch
Description: Binary data
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
