[PATCH]: Implementation of -Wcast-qual

[David Blaikie]

Yep. Though the diagnostic for dropping const and volatile is a bit off
("drops const volatile qualifier" should probably read "drops const and
volatile qualifiers"? (maybe you'll need a %select for this - you could
probably use a %select for the const and volatile separately too if you
like)). What does GCC do here? Does it warn on dropping volatile at all?

Chatting to Richard over lunch he mentioned an interesting case where we
might want to warn:

  int **x;
  auto y = (const int **)x;

which, if it were a static_cast, would warn:

  cast.cpp:2:10: error: static_cast from 'int **' to 'const int **' is not
allowed
  auto y = static_cast<const int **>(x);
           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~

I'm just not sure we'll be able to get a good diagnostic in both these
cases. But as I think/type this out I think:

We should just use the same machinery that powers static_cast here,
(Richard mentioned there should be a way to test whether a conversion is a
"qualification conversion" which is the description of the valid implicit
qualification changes, and not the above const-changing cases)) and we
should teach the machinery to give us enough information to create good
diagnostics - telling the user where the missing const, volatile, etc, is.

Sorry to go through so many iterations - it didn't occur to me until
Richard mentioned it that there was this more general approach.

(wonder what GCC does here? - hmm, looks like it gets the "int** -> const
int**" right: cast from type ‘int**’ to type ‘const int**’ casts away
qualifiers)

On Fri, Nov 14, 2014 at 12:02 PM, Roman Divacky <rdivacky at vlakno.cz> wrote:

> Like this?
>
> On Fri, Nov 14, 2014 at 11:20:49AM -0800, David Blaikie wrote:
> > 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/db282838/attachment.html>