Thanks for review, Dmitri! Updated patch attached. On 23 January 2013 23:52, Dmitri Gribenko <[email protected]> wrote:
> On Wed, Jan 23, 2013 at 11:33 PM, Alexander Zinenko <[email protected]> > wrote: > > Thanks for feedback! Here is an updated version. It warns by default in > case > > of virtual base or in case of nonzero offset. Other up/downcasts lead to > a > > warning with -Wreinterpret-updown-extra, ignored by default. > > I think it is sensible to leave the warning on by default. The > purpose of a separate flag is to turn off the (possibly) noisy case > separately. > Having it on by default, clang fails to pass these tests: Clang :: Analysis/inlining/dyn-dispatch-bifurcate.cpp Clang :: SemaCXX/address-space-conversion.cpp That's why the noisy version was disabled by default even in the first version. > > I am not a fan of the name '-Wreinterpret-updown-extra'. > -Wreinterpret-updown-zero-adjustment or -Wreinterpret-updown-safe > might be better. > Changed to -Wreinterpret-updown-zero-adjustment. -Wreinterpret-updown-safe is a bit misleading: why warn about safe things? > > + // subobject with offset > > Comments should start with a capital letter and end with a period. > > +/// ReinterpretUpDownCast - Check that a > reinterpret_cast\<DestType\>(SrcExpr) > +/// is not used as upcast or downcast between respective pointers or > +/// references. > > Please don't duplicate function name in comments when adding new code. > > +static void ReinterpretUpDownCast(Sema &Self, ExprResult &SrcExpr, > > A better name: DiagnoseReinterpretUpDownCast? > > + CharUnits offset = CharUnits::Zero(); > > s/offset/Offset/ > > +def warn_reinterpret_updowncast : Warning< > + "'reinterpret_cast' is used as an %select{upcast|downcast}0 from type > %1 " > + "to its %select{base|derived}0 type %2">, > > (1) Trailing whitespace. > > (2) It would be better to do "%select{an upcast|a downcast}" or we > would print "an downcast". > > +def warn_reinterpret_wrong_subobject : Warning< > + "'reinterpret_cast' might result in pointing to wrong subobject">, > > ... might return a pointer... ? > > +def note_reinterpret_updowncast_use_static_cast : Note< > + "use of 'static_cast' adjusts the pointer correctly while " > + "%select{upcasting|downcasting}0">; > > "use 'static_cast' to adjust the pointer correctly ..." ? > > +def note_reinterpret_virtual_base : Note< > + "casting %select{to|from}0 virtual base type %1 %select{from|to}0 " > + "its derived type %2">, InGroup<ReinterpretUpDownCast>; > > Notes should not be put in groups. > > + InGroup<ReinterpretUpDownCast>, DefaultWarn; > > DefaultWarn can be dropped here. > Fixed. > > Dmitri > > -- > main(i,j){for(i=2;;i++){for(j=2;j<i;j++){if(!(i%j)){j=0;break;}}if > (j){printf("%d\n",i);}}} /*Dmitri Gribenko <[email protected]>*/ >
13824.patch
Description: Binary data
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
