[cfe-commits] Patch: Add warning for undefined reinterpret_cast behavior

[Richard Trieu]

Revising the patch per your suggestions:
1) Make this warning off by default
2) Removing unrelated whitespace changes
3) Replace underscores with hyphens in diagnostic group name

On Sun, Mar 27, 2011 at 3:13 AM, John McCall <rjmccall at apple.com> wrote:

> On Mar 27, 2011, at 2:44 AM, Douglas Gregor wrote:
> > On Mar 22, 2011, at 10:24 PM, Richard Trieu wrote:
> >> Add a warning for when reinterpret_cast leads to undefined behavior.
>  Also updated reinterpret_cast tests and turned off this warning for tests
> that don't need it.
> >
> > I'm not totally convinced that this warning is useful. The issue is that
> we're not really invoking undefined behavior by performing the cast, or even
> dereferencing the result of that cast, so long as the memory we're pointing
> to actually has an object of the right type. However, that's not what this
> warning is checking: this warning is checking whether the declared type
> before the cast, and the type we're casting to, are compatible.
> >
> > Overall, I expect that this warning will have a fairly high
> false-positive rate, since programmers tend to use reinterpret_cast in very
> specific ways where they know far more about the type of the object behind
> the pointer/reference than the type system says. To really catch problems
> with reinterpret_cast, we're likely to need some stronger analysis (e.g.,
> something in the static analyzer) that checks that we aren't referring to
> the same lvalue in different places with different, incompatible types.
>
> This isn't just an abstract point — we use reinterpret_cast in various
> Clang headers to convert up and down class hierarchies that we don't want to
> require the inclusion of headers for.  In addition, while reinterpreting a
> pointer to a double object as an int* and accessing it is technically
> undefined behavior because of aliasing rules, it's actually such an
> important idiom that a major design goal of the LLVM aliasing optimization
> has been to make sure that "sufficiently obvious" code is not broken by
> strict aliasing.
>
> I think that even if this warning were doing the more sophisticated
> analysis Doug has in mind, it would still be inappropriate for it to be
> enabled by default.
>
> Also, your patch has unrelated whitespace changes;  please remove these.
>
> John.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20110329/e6a0e955/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: undefined-reinterpret-cast.patch
Type: text/x-patch
Size: 7457 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20110329/e6a0e955/attachment.bin>