[PATCH]: Implementation of -Wcast-qual

[David Blaikie]

I take it this is consistent with the GCC warning - in terms of warning on
the innermost issue, reporting const or volatile - what about dropping
const and volatile at the same time?

Issues with the current code:

* DestPtr and SrcPtr don't need to be initialized to null, they'll be
written to on the first loop iteration as needed - avoiding excess
initialization helps tools like MSan find more bugs rather than the program
silently using unintended default values

* InnerMostDestType and InnerMostSrcType will be dangling pointers after
the while loop (so accessing them in the proceeding 'if' is UB)

* you don't need to check both InnerMostDestType and InnerMostSrcType in
the following if - it's invariant that if one is non-null (you can use
QualType values rather than QualType* to address the previous bug, and use
QualTypes "isNull()" member function here) so is the other

On Fri, Nov 14, 2014 at 11:07 AM, Roman Divacky <rdivacky at vlakno.cz> wrote:

> Actually, try this patch. It includes check for volatile as well.
>
> On Wed, Nov 12, 2014 at 12:39:20PM -0800, David Blaikie wrote:
> > [+Richard for oversight]
> >
> > char **y1 = (char **)ptrptr; // expected-warning {{cast from 'const char
> > *const *' to 'char **' drops const qualifier}} expected-warning {{cast
> from
> > 'const char *const' to 'char *' drops const qualifier}}
> >
> > I think if we're going to warn on multiple layers (I'm not sure that's
> > ideal - is that consistent with GCC's warning? Does GCC warn on
> mismatched
> > types too - "const T*" -> "U*"? - do we warn there too, or only when
> > there's a valid implicit conversion like the void* example?) then we
> should
> > probably drop the top level const, "const char *const" -> "char*" - the
> top
> > level const on the first type is confusing/misleading, it's only relevant
> > to show "const char*" and "char*".
> >
> >
> > On Wed, Nov 12, 2014 at 10:41 AM, Roman Divacky <rdivacky at vlakno.cz>
> wrote:
> >
> > > I expanded the testcase and fixed the grammar in the actual warning.
> > >
> > > New patch attached.
> > >
> > > On Tue, Nov 11, 2014 at 05:03:33PM -0800, David Blaikie wrote:
> > > > (it's a bit easier if you include the test in the same patch file -
> also
> > > > you can use Phabricator if you like - some reviewers perefer it)
> > > >
> > > > Since you've got the loop there for seeing through multiple levels of
> > > > pointer, should you have a test case that exercises that on a > 1
> level
> > > of
> > > > depth? Demonstrate that we warn on both levels (if that's the right
> thing
> > > > to do?)?
> > > >
> > > > Optionally (probably in a separate follow-up patch) you could add a
> note
> > > > with a fixit to include the missing consts.
> > > >
> > > > On Tue, Nov 11, 2014 at 10:58 AM, Roman Divacky <rdivacky at vlakno.cz>
> > > wrote:
> > > >
> > > > > Hi,
> > > > >
> > > > > I implemented -Wcast-qual. The patch is actually quite short
> (attached
> > > + a
> > > > > test
> > > > > case).
> > > > >
> > > > > This fixes #13772 and also note that -Wcast-qual is used in llvm
> build
> > > > > itself.
> > > > >
> > > > > Is this ok to be commited? Thanks
> > > > >
> > > > > Roman
> > > > >
> > > > >
> > > > > _______________________________________________
> > > > > cfe-commits mailing list
> > > > > cfe-commits at cs.uiuc.edu
> > > > > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
> > > > >
> > > > >
> > >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20141114/c0fa9b81/attachment.html>