[PATCH]: Implementation of -Wcast-qual

Tue Nov 18 03:49:59 PST 2014

Perhaps like this?

On Fri, Nov 14, 2014 at 01:17:38PM -0800, David Blaikie wrote:
> 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 --------------
A non-text attachment was scrubbed...
Name: clang-cast-qual.patch
Type: text/x-diff
Size: 6227 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20141118/5c49bc4c/attachment.patch>