[cfe-commits] [PATCH] PR13824 warn if reinterpret_cast used for up/downcast

[Alexander Zinenko]

Thanks for review, Dmitri!
Updated patch attached.

On 23 January 2013 23:52, Dmitri Gribenko <gribozavr at gmail.com> wrote:

> On Wed, Jan 23, 2013 at 11:33 PM, Alexander Zinenko <ftynse at gmail.com>
> 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 <gribozavr at gmail.com>*/
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130124/8c68ab3a/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 13824.patch
Type: application/octet-stream
Size: 27034 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130124/8c68ab3a/attachment.obj>