On Fri, Nov 21, 2014 at 12:14 PM, Roman Divacky <[email protected]> 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 <[email protected]> > 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 <[email protected] > > > > > 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 < > [email protected] > > > > > > > > > > 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 < > > > > > > [email protected]> > > > > > > > > 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 < > > > > > > > > [email protected]> > > > > > > > > > > > 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 < > > > > > > > > [email protected]> > > > > > > > > > > > > > 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 < > > > > > > > > > > > [email protected]> > > > > > > > > > > > > > > > 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 > > > > > > > > > > > > > > > > > [email protected] > > > > > > > > > > > > > > > > > > > > > > > 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}} > > > > +} > > > > > > >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
