[PATCH]: Implementation of -Wcast-qual
David Blaikie
dblaikie at gmail.com
Fri Nov 21 12:30:57 PST 2014
On Fri, Nov 21, 2014 at 12:14 PM, Roman Divacky <rdivacky at vlakno.cz> wrote:
> Removed the return using your suggestion. Kept the warn_cast_qual as I dont
> have a better name plus it _is_ triggered by -Wcast-qual.
>
> Good to go?
>
Sure, good enough for any follow-up from Richard to go in post-commit, I
think.
Thanks,
- David
>
> On Fri, Nov 21, 2014 at 11:59:24AM -0800, David Blaikie wrote:
> > Richard - is this in the ballpark of what you had in mind? It's looking
> > about right to me.
> >
> > Looking pretty good to me.
> >
> > That 'return' just after the diag::warn_cast_qual2 might catch someone by
> > surprise when they add a new chunk of warning handling for some new
> warning
> > at the end of this function (only to find their new warning doesn't fire
> if
> > the user uses this /particular/ kind of bad cast... ). Perhaps we could
> > avoid that?
> >
> > Maybe initialize qualifiers to -1, then just do:
> >
> > if (qualifiers != -1)
> > Diag(warn_cast_qual)
> > else
> > Diag(warn_cast_qual2)
> >
> > (& is there a better name for cast_qual2? I don't mind too much)
> >
> > On Thu, Nov 20, 2014 at 4:57 AM, Roman Divacky <rdivacky at vlakno.cz>
> wrote:
> >
> > > Now a patch that actually survives clang tests with assertions enabled.
> > >
> > > On Thu, Nov 20, 2014 at 10:41:28AM +0100, Roman Divacky wrote:
> > > > Patch hopefully addressing all the concerns attached.
> > > >
> > > > On Wed, Nov 19, 2014 at 10:05:37AM -0800, David Blaikie wrote:
> > > > > On Tue, Nov 18, 2014 at 2:05 PM, Roman Divacky <rdivacky at vlakno.cz
> >
> > > wrote:
> > > > >
> > > > > > New patch attached. I've checked the behaviour against gcc5 and
> we
> > > warn on
> > > > > > the
> > > > > > same cases.
> > > > > >
> > > > >
> > > > > Great! Thanks so much for your patience & diligence. Does GCC
> mention
> > > the
> > > > > same types as Clang? (I notice in some of your examples... oh, you
> > > don't
> > > > > have such an example. What does GCC and Clang do for "const char
> **" ->
> > > > > "char **"? It looks like, judging by the "int**" -> "const int **"
> > > warning,
> > > > > we might print just the inner type there as well ("cast from const
> > > char* to
> > > > > char*") - is that what GCC does? Or does it print the full outer
> types
> > > > > always? Is your patch's behavior going to be better for users
> > > (probably,
> > > > > but I'm not sure).
> > > > >
> > > > >
> > > > > > For the const int **bahc = (const int **)bah; case gcc5 emits a
> > > warning
> > > > > > with
> > > > > > a different warning (to be safe all intermediate pointers in cast
> > > from
> > > > > > 'int **'
> > > > > > to 'volatile int **' must be 'const' qualified [-Wcast-qual]). Do
> > > you think
> > > > > > it's worth adding similarly worded warning for that case or is
> the
> > > > > > "dropping
> > > > > > const" ok?
> > > > > >
> > > > >
> > > > > I think it's OK, but confusing - it'd be good to make it better,
> but
> > > > > possibly as a follow-up patch. Could you include FIXMEs in the test
> > > case
> > > > > for both the const and volatile cases for this showing what GCC
> does
> > > (or
> > > > > otherwise suggesting better wording). I'm assuming in even the
> volatile
> > > > > case, the correct (and GCC5) text is that you need to add const on
> the
> > > > > intermediate pointers?
> > > > >
> > > > > Actually, now that I look at the test case, it seems a bit worse -
> > > it's not
> > > > > printing the top-level type in this case (int**->const int**) it's
> > > printing
> > > > > the nested type (int*->const int*) and saying it drops const,
> which is
> > > even
> > > > > more confusing... hrm. So maybe it makes sense to fix this in the
> > > current
> > > > > patch rather than a follow-up, given how confusing it is.
> > > > >
> > > > > One thing to do with this current patch, would be to use a %select
> > > rather
> > > > > than %2 and %3 being directly substituted (having to add that 's'
> is a
> > > bit
> > > > > weird). Something like this:
> > > > >
> > > > > Warning<"cast from %0 to %1 drops %select{const and volatile
> > > > > qualifiers|const qualifier|volatile qualifier}2">
> > > > >
> > > > > int qualifiers = 0;
> > > > > if (const && volatile)
> > > > > qualifiers = 0;
> > > > > else if (const)
> > > > > qualifiers = 1;
> > > > > else if (volatile)
> > > > > qualifiers = 2;
> > > > >
> > > > > etc.
> > > > >
> > > > > & doing the better "all intermediate pointers in cast" thing
> shouldn't
> > > be
> > > > > too hard, right? Just another warning in the .td file, plus a
> condition
> > > > > before all that to say "if !const && !volatile <do the other
> warning>"
> > > but
> > > > > you'll need to get those types right (providing the top level
> types,
> > > not
> > > > > the nested types, especially in that instance).
> > > > >
> > > > >
> > > > >
> > > > >
> > > > > >
> > > > > > On Tue, Nov 18, 2014 at 09:27:17AM -0800, David Blaikie wrote:
> > > > > > > The last line should have a warning on it (and assuming the
> > > > > > > expected-warning comments in the test are correct, it doesn't):
> > > > > > >
> > > > > > > GCC 4.9:
> > > > > > >
> > > > > > > warning: cast from type 'int**' to type 'const int**' casts
> away
> > > > > > qualifiers
> > > > > > > [-Wcast-qual]
> > > > > > > const int **bahc = (const int **)bah;
> > > > > > >
> > > > > > > On Tue, Nov 18, 2014 at 6:11 AM, Roman Divacky <
> rdivacky at vlakno.cz
> > > >
> > > > > > wrote:
> > > > > > >
> > > > > > > > Scrap the previous patch. It doesnt work when we add
> qualifiers.
> > > The
> > > > > > new
> > > > > > > > version is fixed.
> > > > > > > >
> > > > > > > > On Tue, Nov 18, 2014 at 12:49:59PM +0100, Roman Divacky
> wrote:
> > > > > > > > > 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
> > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > >
> > > > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > >
> > > > > > > > > Index: include/clang/Basic/DiagnosticGroups.td
> > > > > > > > >
> > > ===================================================================
> > > > > > > > > --- include/clang/Basic/DiagnosticGroups.td (revision
> 222228)
> > > > > > > > > +++ include/clang/Basic/DiagnosticGroups.td (working
> copy)
> > > > > > > > > @@ -60,7 +60,7 @@
> > > > > > > > > def KeywordCompat : DiagGroup<"keyword-compat">;
> > > > > > > > > def GNUCaseRange : DiagGroup<"gnu-case-range">;
> > > > > > > > > def CastAlign : DiagGroup<"cast-align">;
> > > > > > > > > -def : DiagGroup<"cast-qual">;
> > > > > > > > > +def CastQual : DiagGroup<"cast-qual">;
> > > > > > > > > def : DiagGroup<"char-align">;
> > > > > > > > > def Comment : DiagGroup<"comment">;
> > > > > > > > > def GNUComplexInteger : DiagGroup<"gnu-complex-integer">;
> > > > > > > > > Index: include/clang/Basic/DiagnosticSemaKinds.td
> > > > > > > > >
> > > ===================================================================
> > > > > > > > > --- include/clang/Basic/DiagnosticSemaKinds.td
> (revision
> > > > > > 222228)
> > > > > > > > > +++ include/clang/Basic/DiagnosticSemaKinds.td
> (working
> > > copy)
> > > > > > > > > @@ -6104,6 +6104,8 @@
> > > > > > > > > def warn_zero_size_struct_union_in_extern_c :
> > > > > > Warning<"%select{|empty
> > > > > > > > }0"
> > > > > > > > > "%select{struct|union}1 has size 0 in C, %select{size
> > > 1|non-zero
> > > > > > > > size}2 in C++">,
> > > > > > > > > InGroup<ExternCCompat>;
> > > > > > > > > +def warn_cast_qual : Warning<"cast from %0 to %1 drops %2
> > > > > > qualifier%3">,
> > > > > > > > > + InGroup<CastQual>, DefaultIgnore;
> > > > > > > > > } // End of general sema category.
> > > > > > > > >
> > > > > > > > > // inline asm.
> > > > > > > > > Index: lib/Sema/SemaCast.cpp
> > > > > > > > >
> > > ===================================================================
> > > > > > > > > --- lib/Sema/SemaCast.cpp (revision 222228)
> > > > > > > > > +++ lib/Sema/SemaCast.cpp (working copy)
> > > > > > > > > @@ -143,7 +143,10 @@
> > > > > > > > > }
> > > > > > > > >
> > > > > > > > > static bool CastsAwayConstness(Sema &Self, QualType
> SrcType,
> > > > > > QualType
> > > > > > > > DestType,
> > > > > > > > > - bool CheckCVR, bool
> > > > > > CheckObjCLifetime);
> > > > > > > > > + bool CheckCVR, bool
> > > > > > CheckObjCLifetime,
> > > > > > > > > + QualType
> *TheOffendingSrcType =
> > > > > > nullptr,
> > > > > > > > > + QualType
> *TheOffendingDestType
> > > =
> > > > > > nullptr,
> > > > > > > > > + Qualifiers
> *CastAwayQualifiers
> > > =
> > > > > > > > nullptr);
> > > > > > > > >
> > > > > > > > > // The Try functions attempt a specific way of casting. If
> > > they
> > > > > > > > succeed, they
> > > > > > > > > // return TC_Success. If their way of casting is not
> > > appropriate for
> > > > > > > > the given
> > > > > > > > > @@ -462,7 +465,10 @@
> > > > > > > > > /// \param CheckObjCLifetime Whether to check Objective-C
> > > lifetime
> > > > > > > > qualifiers.
> > > > > > > > > static bool
> > > > > > > > > CastsAwayConstness(Sema &Self, QualType SrcType, QualType
> > > DestType,
> > > > > > > > > - bool CheckCVR, bool CheckObjCLifetime)
> {
> > > > > > > > > + bool CheckCVR, bool CheckObjCLifetime,
> > > > > > > > > + QualType *TheOffendingSrcType,
> > > > > > > > > + QualType *TheOffendingDestType,
> > > > > > > > > + Qualifiers *CastAwayQualifiers) {
> > > > > > > > > // If the only checking we care about is for Objective-C
> > > lifetime
> > > > > > > > qualifiers,
> > > > > > > > > // and we're not in ARC mode, there's nothing to check.
> > > > > > > > > if (!CheckCVR && CheckObjCLifetime &&
> > > > > > > > > @@ -487,6 +493,8 @@
> > > > > > > > > // Find the qualifiers. We only care about
> cvr-qualifiers
> > > for the
> > > > > > > > > // purpose of this check, because other qualifiers
> (address
> > > > > > spaces,
> > > > > > > > > // Objective-C GC, etc.) are part of the type's
> identity.
> > > > > > > > > + QualType PrevUnwrappedSrcType = UnwrappedSrcType;
> > > > > > > > > + QualType PrevUnwrappedDestType = UnwrappedDestType;
> > > > > > > > > while (UnwrapDissimilarPointerTypes(UnwrappedSrcType,
> > > > > > > > UnwrappedDestType)) {
> > > > > > > > > // Determine the relevant qualifiers at this level.
> > > > > > > > > Qualifiers SrcQuals, DestQuals;
> > > > > > > > > @@ -497,6 +505,13 @@
> > > > > > > > > if (CheckCVR) {
> > > > > > > > >
> > > > > > RetainedSrcQuals.setCVRQualifiers(SrcQuals.getCVRQualifiers());
> > > > > > > > >
> > > > > > RetainedDestQuals.setCVRQualifiers(DestQuals.getCVRQualifiers());
> > > > > > > > > +
> > > > > > > > > + if (RetainedSrcQuals != RetainedDestQuals &&
> > > > > > TheOffendingSrcType
> > > > > > > > &&
> > > > > > > > > + TheOffendingDestType && CastAwayQualifiers) {
> > > > > > > > > + *TheOffendingSrcType = PrevUnwrappedSrcType;
> > > > > > > > > + *TheOffendingDestType = PrevUnwrappedDestType;
> > > > > > > > > + *CastAwayQualifiers = RetainedSrcQuals -
> > > RetainedDestQuals;
> > > > > > > > > + }
> > > > > > > > > }
> > > > > > > > >
> > > > > > > > > if (CheckObjCLifetime &&
> > > > > > > > > @@ -505,6 +520,9 @@
> > > > > > > > >
> > > > > > > > > cv1.push_back(RetainedSrcQuals);
> > > > > > > > > cv2.push_back(RetainedDestQuals);
> > > > > > > > > +
> > > > > > > > > + PrevUnwrappedSrcType = UnwrappedSrcType;
> > > > > > > > > + PrevUnwrappedDestType = UnwrappedDestType;
> > > > > > > > > }
> > > > > > > > > if (cv1.empty())
> > > > > > > > > return false;
> > > > > > > > > @@ -2371,6 +2389,28 @@
> > > > > > > > >
> > > > > > > > > if (Kind == CK_BitCast)
> > > > > > > > > checkCastAlign();
> > > > > > > > > +
> > > > > > > > > + // -Wcast-qual
> > > > > > > > > + QualType TheOffendingSrcType, TheOffendingDestType;
> > > > > > > > > + Qualifiers CastAwayQualifiers;
> > > > > > > > > + if (CastsAwayConstness(Self, SrcType, DestType, true,
> false,
> > > > > > > > > + &TheOffendingSrcType,
> > > &TheOffendingDestType,
> > > > > > > > > + &CastAwayQualifiers)) {
> > > > > > > > > + const char *qualifiers;
> > > > > > > > > + const char *suffix = "";
> > > > > > > > > + if (CastAwayQualifiers.hasConst() &&
> > > > > > > > CastAwayQualifiers.hasVolatile()) {
> > > > > > > > > + qualifiers = "const and volatile";
> > > > > > > > > + suffix = "s";
> > > > > > > > > + } else if (CastAwayQualifiers.hasConst())
> > > > > > > > > + qualifiers = "const";
> > > > > > > > > + else if (CastAwayQualifiers.hasVolatile())
> > > > > > > > > + qualifiers = "volatile";
> > > > > > > > > + else {
> > > > > > > > > + llvm_unreachable("Impossible qualifier");
> > > > > > > > > + }
> > > > > > > > > + Self.Diag(SrcExpr.get()->getLocStart(),
> > > diag::warn_cast_qual) <<
> > > > > > > > > + TheOffendingSrcType << TheOffendingDestType <<
> > > qualifiers <<
> > > > > > > > suffix;
> > > > > > > > > + }
> > > > > > > > > }
> > > > > > > > >
> > > > > > > > > ExprResult Sema::BuildCStyleCastExpr(SourceLocation LPLoc,
> > > > > > > > > Index: test/Sema/warn-cast-qual.c
> > > > > > > > >
> > > ===================================================================
> > > > > > > > > --- test/Sema/warn-cast-qual.c (revision 0)
> > > > > > > > > +++ test/Sema/warn-cast-qual.c (working copy)
> > > > > > > > > @@ -0,0 +1,19 @@
> > > > > > > > > +// RUN: %clang_cc1 -triple x86_64-unknown-unknown
> > > -fsyntax-only
> > > > > > > > -Wcast-qual -verify %s
> > > > > > > > > +
> > > > > > > > > +#include <stdint.h>
> > > > > > > > > +
> > > > > > > > > +void foo() {
> > > > > > > > > + const char * const ptr = 0;
> > > > > > > > > + const char * const *ptrptr = 0;
> > > > > > > > > + char *y = (char *)ptr; // expected-warning {{cast
> from
> > > 'const
> > > > > > > > char *' to 'char *' drops const qualifier}}
> > > > > > > > > + char **y1 = (char **)ptrptr; // expected-warning
> > > {{cast from
> > > > > > > > 'const char *const' to 'char *' drops const qualifier}}
> > > > > > > > > + const char **y2 = (const char **)ptrptr; //
> > > expected-warning
> > > > > > {{cast
> > > > > > > > from 'const char *const *' to 'const char **' drops const
> > > qualifier}}
> > > > > > > > > +
> > > > > > > > > + char *z = (char *)(uintptr_t)(const void *)ptr; // no
> > > warning
> > > > > > > > > + char *z1 = (char *)(const void *)ptr; //
> > > expected-warning
> > > > > > {{cast
> > > > > > > > from 'const void *' to 'char *' drops const qualifier}}
> > > > > > > > > +
> > > > > > > > > + volatile char *vol = 0;
> > > > > > > > > + char *vol2 = (char *)vol; // expected-warning {{cast
> from
> > > > > > 'volatile
> > > > > > > > char *' to 'char *' drops volatile qualifier}}
> > > > > > > > > + const volatile char *volc = 0;
> > > > > > > > > + char *volc2 = (char *)volc; // expected-warning {{cast
> from
> > > 'const
> > > > > > > > volatile char *' to 'char *' drops const and volatile
> > > qualifiers}}
> > > > > > > > > +}
> > > > > > > >
> > > > > > > >
> > > > > >
> > >
> > > > Index: include/clang/Basic/DiagnosticGroups.td
> > > > ===================================================================
> > > > --- include/clang/Basic/DiagnosticGroups.td (revision 222421)
> > > > +++ include/clang/Basic/DiagnosticGroups.td (working copy)
> > > > @@ -60,7 +60,7 @@
> > > > def KeywordCompat : DiagGroup<"keyword-compat">;
> > > > def GNUCaseRange : DiagGroup<"gnu-case-range">;
> > > > def CastAlign : DiagGroup<"cast-align">;
> > > > -def : DiagGroup<"cast-qual">;
> > > > +def CastQual : DiagGroup<"cast-qual">;
> > > > def : DiagGroup<"char-align">;
> > > > def Comment : DiagGroup<"comment">;
> > > > def GNUComplexInteger : DiagGroup<"gnu-complex-integer">;
> > > > Index: include/clang/Basic/DiagnosticSemaKinds.td
> > > > ===================================================================
> > > > --- include/clang/Basic/DiagnosticSemaKinds.td (revision
> 222421)
> > > > +++ include/clang/Basic/DiagnosticSemaKinds.td (working copy)
> > > > @@ -6112,6 +6112,11 @@
> > > > def warn_zero_size_struct_union_in_extern_c :
> Warning<"%select{|empty
> > > }0"
> > > > "%select{struct|union}1 has size 0 in C, %select{size 1|non-zero
> > > size}2 in C++">,
> > > > InGroup<ExternCCompat>;
> > > > +def warn_cast_qual : Warning<"cast from %0 to %1 drops %select{const
> > > and"
> > > > + "volatile qualifiers|const qualifier|volatile qualifier}2">,
> > > > + InGroup<CastQual>, DefaultIgnore;
> > > > +def warn_cast_qual2 : Warning<"cast from %0 to %1 must have all
> > > intermediate "
> > > > + "pointers const qualified to be safe">, InGroup<CastQual>,
> > > DefaultIgnore;
> > > > } // End of general sema category.
> > > >
> > > > // inline asm.
> > > > Index: lib/Sema/SemaCast.cpp
> > > > ===================================================================
> > > > --- lib/Sema/SemaCast.cpp (revision 222421)
> > > > +++ lib/Sema/SemaCast.cpp (working copy)
> > > > @@ -142,9 +142,6 @@
> > > > };
> > > > }
> > > >
> > > > -static bool CastsAwayConstness(Sema &Self, QualType SrcType,
> QualType
> > > DestType,
> > > > - bool CheckCVR, bool
> CheckObjCLifetime);
> > > > -
> > > > // The Try functions attempt a specific way of casting. If they
> > > succeed, they
> > > > // return TC_Success. If their way of casting is not appropriate for
> > > the given
> > > > // arguments, they return TC_NotApplicable and *may* set diag to a
> > > diagnostic
> > > > @@ -462,7 +459,10 @@
> > > > /// \param CheckObjCLifetime Whether to check Objective-C lifetime
> > > qualifiers.
> > > > static bool
> > > > CastsAwayConstness(Sema &Self, QualType SrcType, QualType DestType,
> > > > - bool CheckCVR, bool CheckObjCLifetime) {
> > > > + bool CheckCVR, bool CheckObjCLifetime,
> > > > + QualType *TheOffendingSrcType = nullptr,
> > > > + QualType *TheOffendingDestType = nullptr,
> > > > + Qualifiers *CastAwayQualifiers = nullptr) {
> > > > // If the only checking we care about is for Objective-C lifetime
> > > qualifiers,
> > > > // and we're not in ARC mode, there's nothing to check.
> > > > if (!CheckCVR && CheckObjCLifetime &&
> > > > @@ -487,6 +487,8 @@
> > > > // Find the qualifiers. We only care about cvr-qualifiers for the
> > > > // purpose of this check, because other qualifiers (address
> spaces,
> > > > // Objective-C GC, etc.) are part of the type's identity.
> > > > + QualType PrevUnwrappedSrcType = UnwrappedSrcType;
> > > > + QualType PrevUnwrappedDestType = UnwrappedDestType;
> > > > while (UnwrapDissimilarPointerTypes(UnwrappedSrcType,
> > > UnwrappedDestType)) {
> > > > // Determine the relevant qualifiers at this level.
> > > > Qualifiers SrcQuals, DestQuals;
> > > > @@ -497,6 +499,13 @@
> > > > if (CheckCVR) {
> > > >
> RetainedSrcQuals.setCVRQualifiers(SrcQuals.getCVRQualifiers());
> > > >
> RetainedDestQuals.setCVRQualifiers(DestQuals.getCVRQualifiers());
> > > > +
> > > > + if (RetainedSrcQuals != RetainedDestQuals &&
> TheOffendingSrcType
> > > &&
> > > > + TheOffendingDestType && CastAwayQualifiers) {
> > > > + *TheOffendingSrcType = PrevUnwrappedSrcType;
> > > > + *TheOffendingDestType = PrevUnwrappedDestType;
> > > > + *CastAwayQualifiers = RetainedSrcQuals - RetainedDestQuals;
> > > > + }
> > > > }
> > > >
> > > > if (CheckObjCLifetime &&
> > > > @@ -505,6 +514,9 @@
> > > >
> > > > cv1.push_back(RetainedSrcQuals);
> > > > cv2.push_back(RetainedDestQuals);
> > > > +
> > > > + PrevUnwrappedSrcType = UnwrappedSrcType;
> > > > + PrevUnwrappedDestType = UnwrappedDestType;
> > > > }
> > > > if (cv1.empty())
> > > > return false;
> > > > @@ -2371,6 +2383,29 @@
> > > >
> > > > if (Kind == CK_BitCast)
> > > > checkCastAlign();
> > > > +
> > > > + // -Wcast-qual
> > > > + QualType TheOffendingSrcType, TheOffendingDestType;
> > > > + Qualifiers CastAwayQualifiers;
> > > > + if (CastsAwayConstness(Self, SrcType, DestType, true, false,
> > > > + &TheOffendingSrcType,
> &TheOffendingDestType,
> > > > + &CastAwayQualifiers)) {
> > > > + int qualifiers = 0;
> > > > + if (CastAwayQualifiers.hasConst() &&
> > > CastAwayQualifiers.hasVolatile()) {
> > > > + qualifiers = 0;
> > > > + } else if (CastAwayQualifiers.hasConst()) {
> > > > + qualifiers = 1;
> > > > + } else if (CastAwayQualifiers.hasVolatile()) {
> > > > + qualifiers = 2;
> > > > + } else {
> > > > + // This is a variant of int **x; const int **y = (const int
> **)x;
> > > > + Self.Diag(SrcExpr.get()->getLocStart(),
> diag::warn_cast_qual2) <<
> > > > + SrcType << DestType;
> > > > + return;
> > > > + }
> > > > + Self.Diag(SrcExpr.get()->getLocStart(), diag::warn_cast_qual) <<
> > > > + TheOffendingSrcType << TheOffendingDestType << qualifiers;
> > > > + }
> > > > }
> > > >
> > > > ExprResult Sema::BuildCStyleCastExpr(SourceLocation LPLoc,
> > > > Index: test/Sema/warn-cast-qual.c
> > > > ===================================================================
> > > > --- test/Sema/warn-cast-qual.c (revision 0)
> > > > +++ test/Sema/warn-cast-qual.c (working copy)
> > > > @@ -0,0 +1,29 @@
> > > > +// RUN: %clang_cc1 -triple x86_64-unknown-unknown -fsyntax-only
> > > -Wcast-qual -verify %s
> > > > +
> > > > +#include <stdint.h>
> > > > +
> > > > +void foo() {
> > > > + const char * const ptr = 0;
> > > > + const char * const *ptrptr = 0;
> > > > + char *y = (char *)ptr; // expected-warning {{cast from 'const
> > > char *' to 'char *' drops const qualifier}}
> > > > + char **y1 = (char **)ptrptr; // expected-warning {{cast from
> > > 'const char *const' to 'char *' drops const qualifier}}
> > > > + const char **y2 = (const char **)ptrptr; // expected-warning
> {{cast
> > > from 'const char *const *' to 'const char **' drops const qualifier}}
> > > > +
> > > > + char *z = (char *)(uintptr_t)(const void *)ptr; // no warning
> > > > + char *z1 = (char *)(const void *)ptr; // expected-warning
> {{cast
> > > from 'const void *' to 'char *' drops const qualifier}}
> > > > +
> > > > + volatile char *vol = 0;
> > > > + char *vol2 = (char *)vol; // expected-warning {{cast from
> 'volatile
> > > char *' to 'char *' drops volatile qualifier}}
> > > > + const volatile char *volc = 0;
> > > > + char *volc2 = (char *)volc; // expected-warning {{cast from 'const
> > > volatile char *' to 'char *' drops const and volatile qualifiers}}
> > > > +
> > > > + int **intptrptr;
> > > > + const int **intptrptrc = (const int **)intptrptr; //
> expected-warning
> > > {{cast from 'int **' to 'const int **' must have all intermediate
> pointers
> > > const qualified}}
> > > > + volatile int **intptrptrv = (volatile int **)intptrptr; //
> > > expected-warning {{cast from 'int **' to 'volatile int **' must have
> all
> > > intermediate pointers const qualified}}
> > > > +
> > > > + int *intptr;
> > > > + const int *intptrc = (const int *)intptr; // no warning
> > > > +
> > > > + const char **charptrptrc;
> > > > + char **charptrptr = (char **)charptrptrc; // expected-warning
> {{cast
> > > from 'const char *' to 'char *' drops const qualifier}}
> > > > +}
> > >
> > >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20141121/741ac9ef/attachment.html>
More information about the cfe-commits
mailing list