[PATCH] D33826: [clang-tidy] avoid pointer cast to more strict alignment check

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

In https://reviews.llvm.org/D33826#772676, @aaron.ballman wrote:

> How does this check compare with the -Wcast-align diagnostic in the Clang frontend? I believe that warning already covers these cases, so I'm wondering what extra value is added with this check?




In https://reviews.llvm.org/D33826#856887, @rjmccall wrote:

> In https://reviews.llvm.org/D33826#856652, @lebedev.ri wrote:
>
> > In https://reviews.llvm.org/D33826#856619, @Eugene.Zelenko wrote:
> >
> > > In https://reviews.llvm.org/D33826#856610, @lebedev.ri wrote:
> > >
> > > > Any status update here? :)
> > > >  I generally do see a benefit in this check, because `-Wcast-align` (at least currently?) does not warn on `reinterpret_cast<>()`.
> > >
> > >
> > > I think will be good idea to extend -Wcast-align.
> >
> >
> > Hm, are you *sure* `reinterpret_cast<>()` is not allowed to increase alignment?
> >  There was a commit that intentionally removed that warning: (by @rjmccall, i believe)
> >  https://github.com/llvm-mirror/clang/commit/35a38d95da89d48778019c37b5f8c9a20f7e309c
>
>
> One of the fundamental design principles to keep in mind when implementing warnings like -Wcast-align is that we're trying to warn users about having written something dangerous, not scold them for writing code a particular way.  Users do need to do these casts sometimes, and there has to be some way of doing them without a warning.  So the question of when to warn has to consider whether the code is explicitly acknowledging the danger.  It would, for example, be a mistake to warn in C about double-casts through (void*), because that is not something that people do accidentally; it is very likely that it is an attempt to suppress the warning.  In C++, it seemed to me that a reinterpret_cast is by its nature explicitly acknowledging the danger, so it is never really appropriate to warn.


So we are back from where we have started, and this check does make sense.


Repository:
  rL LLVM

https://reviews.llvm.org/D33826