<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Nov 21, 2014 at 12:14 PM, Roman Divacky <span dir="ltr"><<a href="mailto:rdivacky@vlakno.cz" target="_blank">rdivacky@vlakno.cz</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Removed the return using your suggestion. Kept the warn_cast_qual as I dont<br>
have a better name plus it _is_ triggered by -Wcast-qual.<br>
<br>
Good to go?<br></blockquote><div><br>Sure, good enough for any follow-up from Richard to go in post-commit, I think.<br><br>Thanks,<br>- David<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="HOEnZb"><div class="h5"><br>
On Fri, Nov 21, 2014 at 11:59:24AM -0800, David Blaikie wrote:<br>
> Richard - is this in the ballpark of what you had in mind? It's looking<br>
> about right to me.<br>
><br>
> Looking pretty good to me.<br>
><br>
> That 'return' just after the diag::warn_cast_qual2 might catch someone by<br>
> surprise when they add a new chunk of warning handling for some new warning<br>
> at the end of this function (only to find their new warning doesn't fire if<br>
> the user uses this /particular/ kind of bad cast... ). Perhaps we could<br>
> avoid that?<br>
><br>
> Maybe initialize qualifiers to -1, then just do:<br>
><br>
> if (qualifiers != -1)<br>
> Diag(warn_cast_qual)<br>
> else<br>
> Diag(warn_cast_qual2)<br>
><br>
> (& is there a better name for cast_qual2? I don't mind too much)<br>
><br>
> On Thu, Nov 20, 2014 at 4:57 AM, Roman Divacky <<a href="mailto:rdivacky@vlakno.cz">rdivacky@vlakno.cz</a>> wrote:<br>
><br>
> > Now a patch that actually survives clang tests with assertions enabled.<br>
> ><br>
> > On Thu, Nov 20, 2014 at 10:41:28AM +0100, Roman Divacky wrote:<br>
> > > Patch hopefully addressing all the concerns attached.<br>
> > ><br>
> > > On Wed, Nov 19, 2014 at 10:05:37AM -0800, David Blaikie wrote:<br>
> > > > On Tue, Nov 18, 2014 at 2:05 PM, Roman Divacky <<a href="mailto:rdivacky@vlakno.cz">rdivacky@vlakno.cz</a>><br>
> > wrote:<br>
> > > ><br>
> > > > > New patch attached. I've checked the behaviour against gcc5 and we<br>
> > warn on<br>
> > > > > the<br>
> > > > > same cases.<br>
> > > > ><br>
> > > ><br>
> > > > Great! Thanks so much for your patience & diligence. Does GCC mention<br>
> > the<br>
> > > > same types as Clang? (I notice in some of your examples... oh, you<br>
> > don't<br>
> > > > have such an example. What does GCC and Clang do for "const char **" -><br>
> > > > "char **"? It looks like, judging by the "int**" -> "const int **"<br>
> > warning,<br>
> > > > we might print just the inner type there as well ("cast from const<br>
> > char* to<br>
> > > > char*") - is that what GCC does? Or does it print the full outer types<br>
> > > > always? Is your patch's behavior going to be better for users<br>
> > (probably,<br>
> > > > but I'm not sure).<br>
> > > ><br>
> > > ><br>
> > > > > For the const int **bahc = (const int **)bah; case gcc5 emits a<br>
> > warning<br>
> > > > > with<br>
> > > > > a different warning (to be safe all intermediate pointers in cast<br>
> > from<br>
> > > > > 'int **'<br>
> > > > > to 'volatile int **' must be 'const' qualified [-Wcast-qual]). Do<br>
> > you think<br>
> > > > > it's worth adding similarly worded warning for that case or is the<br>
> > > > > "dropping<br>
> > > > > const" ok?<br>
> > > > ><br>
> > > ><br>
> > > > I think it's OK, but confusing - it'd be good to make it better, but<br>
> > > > possibly as a follow-up patch. Could you include FIXMEs in the test<br>
> > case<br>
> > > > for both the const and volatile cases for this showing what GCC does<br>
> > (or<br>
> > > > otherwise suggesting better wording). I'm assuming in even the volatile<br>
> > > > case, the correct (and GCC5) text is that you need to add const on the<br>
> > > > intermediate pointers?<br>
> > > ><br>
> > > > Actually, now that I look at the test case, it seems a bit worse -<br>
> > it's not<br>
> > > > printing the top-level type in this case (int**->const int**) it's<br>
> > printing<br>
> > > > the nested type (int*->const int*) and saying it drops const, which is<br>
> > even<br>
> > > > more confusing... hrm. So maybe it makes sense to fix this in the<br>
> > current<br>
> > > > patch rather than a follow-up, given how confusing it is.<br>
> > > ><br>
> > > > One thing to do with this current patch, would be to use a %select<br>
> > rather<br>
> > > > than %2 and %3 being directly substituted (having to add that 's' is a<br>
> > bit<br>
> > > > weird). Something like this:<br>
> > > ><br>
> > > > Warning<"cast from %0 to %1 drops %select{const and volatile<br>
> > > > qualifiers|const qualifier|volatile qualifier}2"><br>
> > > ><br>
> > > > int qualifiers = 0;<br>
> > > > if (const && volatile)<br>
> > > > qualifiers = 0;<br>
> > > > else if (const)<br>
> > > > qualifiers = 1;<br>
> > > > else if (volatile)<br>
> > > > qualifiers = 2;<br>
> > > ><br>
> > > > etc.<br>
> > > ><br>
> > > > & doing the better "all intermediate pointers in cast" thing shouldn't<br>
> > be<br>
> > > > too hard, right? Just another warning in the .td file, plus a condition<br>
> > > > before all that to say "if !const && !volatile <do the other warning>"<br>
> > but<br>
> > > > you'll need to get those types right (providing the top level types,<br>
> > not<br>
> > > > the nested types, especially in that instance).<br>
> > > ><br>
> > > ><br>
> > > ><br>
> > > ><br>
> > > > ><br>
> > > > > On Tue, Nov 18, 2014 at 09:27:17AM -0800, David Blaikie wrote:<br>
> > > > > > The last line should have a warning on it (and assuming the<br>
> > > > > > expected-warning comments in the test are correct, it doesn't):<br>
> > > > > ><br>
> > > > > > GCC 4.9:<br>
> > > > > ><br>
> > > > > > warning: cast from type 'int**' to type 'const int**' casts away<br>
> > > > > qualifiers<br>
> > > > > > [-Wcast-qual]<br>
> > > > > > const int **bahc = (const int **)bah;<br>
> > > > > ><br>
> > > > > > On Tue, Nov 18, 2014 at 6:11 AM, Roman Divacky <<a href="mailto:rdivacky@vlakno.cz">rdivacky@vlakno.cz</a><br>
> > ><br>
> > > > > wrote:<br>
> > > > > ><br>
> > > > > > > Scrap the previous patch. It doesnt work when we add qualifiers.<br>
> > The<br>
> > > > > new<br>
> > > > > > > version is fixed.<br>
> > > > > > ><br>
> > > > > > > On Tue, Nov 18, 2014 at 12:49:59PM +0100, Roman Divacky wrote:<br>
> > > > > > > > Perhaps like this?<br>
> > > > > > > ><br>
> > > > > > > > On Fri, Nov 14, 2014 at 01:17:38PM -0800, David Blaikie wrote:<br>
> > > > > > > > > Yep. Though the diagnostic for dropping const and volatile<br>
> > is a<br>
> > > > > bit off<br>
> > > > > > > > > ("drops const volatile qualifier" should probably read "drops<br>
> > > > > const and<br>
> > > > > > > > > volatile qualifiers"? (maybe you'll need a %select for this<br>
> > - you<br>
> > > > > could<br>
> > > > > > > > > probably use a %select for the const and volatile separately<br>
> > too<br>
> > > > > if you<br>
> > > > > > > > > like)). What does GCC do here? Does it warn on dropping<br>
> > volatile at<br>
> > > > > > > all?<br>
> > > > > > > > ><br>
> > > > > > > > > Chatting to Richard over lunch he mentioned an interesting<br>
> > case<br>
> > > > > where<br>
> > > > > > > we<br>
> > > > > > > > > might want to warn:<br>
> > > > > > > > ><br>
> > > > > > > > > int **x;<br>
> > > > > > > > > auto y = (const int **)x;<br>
> > > > > > > > ><br>
> > > > > > > > > which, if it were a static_cast, would warn:<br>
> > > > > > > > ><br>
> > > > > > > > > cast.cpp:2:10: error: static_cast from 'int **' to 'const<br>
> > int<br>
> > > > > **' is<br>
> > > > > > > not<br>
> > > > > > > > > allowed<br>
> > > > > > > > > auto y = static_cast<const int **>(x);<br>
> > > > > > > > > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~<br>
> > > > > > > > ><br>
> > > > > > > > > I'm just not sure we'll be able to get a good diagnostic in<br>
> > both<br>
> > > > > these<br>
> > > > > > > > > cases. But as I think/type this out I think:<br>
> > > > > > > > ><br>
> > > > > > > > > We should just use the same machinery that powers<br>
> > static_cast here,<br>
> > > > > > > > > (Richard mentioned there should be a way to test whether a<br>
> > > > > conversion<br>
> > > > > > > is a<br>
> > > > > > > > > "qualification conversion" which is the description of the<br>
> > valid<br>
> > > > > > > implicit<br>
> > > > > > > > > qualification changes, and not the above const-changing<br>
> > cases))<br>
> > > > > and we<br>
> > > > > > > > > should teach the machinery to give us enough information to<br>
> > create<br>
> > > > > good<br>
> > > > > > > > > diagnostics - telling the user where the missing const,<br>
> > volatile,<br>
> > > > > etc,<br>
> > > > > > > is.<br>
> > > > > > > > ><br>
> > > > > > > > > Sorry to go through so many iterations - it didn't occur to<br>
> > me<br>
> > > > > until<br>
> > > > > > > > > Richard mentioned it that there was this more general<br>
> > approach.<br>
> > > > > > > > ><br>
> > > > > > > > > (wonder what GCC does here? - hmm, looks like it gets the<br>
> > "int** -><br>
> > > > > > > const<br>
> > > > > > > > > int**" right: cast from type ???int**??? to type ???const<br>
> > int**???<br>
> > > > > > > casts away<br>
> > > > > > > > > qualifiers)<br>
> > > > > > > > ><br>
> > > > > > > > > On Fri, Nov 14, 2014 at 12:02 PM, Roman Divacky <<br>
> > > > > <a href="mailto:rdivacky@vlakno.cz">rdivacky@vlakno.cz</a>><br>
> > > > > > > wrote:<br>
> > > > > > > > ><br>
> > > > > > > > > > Like this?<br>
> > > > > > > > > ><br>
> > > > > > > > > > On Fri, Nov 14, 2014 at 11:20:49AM -0800, David Blaikie<br>
> > wrote:<br>
> > > > > > > > > > > I take it this is consistent with the GCC warning - in<br>
> > terms of<br>
> > > > > > > warning<br>
> > > > > > > > > > on<br>
> > > > > > > > > > > the innermost issue, reporting const or volatile - what<br>
> > about<br>
> > > > > > > dropping<br>
> > > > > > > > > > > const and volatile at the same time?<br>
> > > > > > > > > > ><br>
> > > > > > > > > > > Issues with the current code:<br>
> > > > > > > > > > ><br>
> > > > > > > > > > > * DestPtr and SrcPtr don't need to be initialized to<br>
> > null,<br>
> > > > > they'll<br>
> > > > > > > be<br>
> > > > > > > > > > > written to on the first loop iteration as needed -<br>
> > avoiding<br>
> > > > > excess<br>
> > > > > > > > > > > initialization helps tools like MSan find more bugs<br>
> > rather<br>
> > > > > than the<br>
> > > > > > > > > > program<br>
> > > > > > > > > > > silently using unintended default values<br>
> > > > > > > > > > ><br>
> > > > > > > > > > > * InnerMostDestType and InnerMostSrcType will be dangling<br>
> > > > > pointers<br>
> > > > > > > after<br>
> > > > > > > > > > > the while loop (so accessing them in the proceeding 'if'<br>
> > is UB)<br>
> > > > > > > > > > ><br>
> > > > > > > > > > > * you don't need to check both InnerMostDestType and<br>
> > > > > > > InnerMostSrcType in<br>
> > > > > > > > > > > the following if - it's invariant that if one is<br>
> > non-null (you<br>
> > > > > can<br>
> > > > > > > use<br>
> > > > > > > > > > > QualType values rather than QualType* to address the<br>
> > previous<br>
> > > > > bug,<br>
> > > > > > > and<br>
> > > > > > > > > > use<br>
> > > > > > > > > > > QualTypes "isNull()" member function here) so is the<br>
> > other<br>
> > > > > > > > > > ><br>
> > > > > > > > > > > On Fri, Nov 14, 2014 at 11:07 AM, Roman Divacky <<br>
> > > > > > > <a href="mailto:rdivacky@vlakno.cz">rdivacky@vlakno.cz</a>><br>
> > > > > > > > > > wrote:<br>
> > > > > > > > > > ><br>
> > > > > > > > > > > > Actually, try this patch. It includes check for<br>
> > volatile as<br>
> > > > > well.<br>
> > > > > > > > > > > ><br>
> > > > > > > > > > > > On Wed, Nov 12, 2014 at 12:39:20PM -0800, David Blaikie<br>
> > > > > wrote:<br>
> > > > > > > > > > > > > [+Richard for oversight]<br>
> > > > > > > > > > > > ><br>
> > > > > > > > > > > > > char **y1 = (char **)ptrptr; // expected-warning<br>
> > {{cast<br>
> > > > > from<br>
> > > > > > > 'const<br>
> > > > > > > > > > char<br>
> > > > > > > > > > > > > *const *' to 'char **' drops const qualifier}}<br>
> > > > > expected-warning<br>
> > > > > > > > > > {{cast<br>
> > > > > > > > > > > > from<br>
> > > > > > > > > > > > > 'const char *const' to 'char *' drops const<br>
> > qualifier}}<br>
> > > > > > > > > > > > ><br>
> > > > > > > > > > > > > I think if we're going to warn on multiple layers<br>
> > (I'm not<br>
> > > > > sure<br>
> > > > > > > > > > that's<br>
> > > > > > > > > > > > > ideal - is that consistent with GCC's warning? Does<br>
> > GCC<br>
> > > > > warn on<br>
> > > > > > > > > > > > mismatched<br>
> > > > > > > > > > > > > types too - "const T*" -> "U*"? - do we warn there<br>
> > too, or<br>
> > > > > > > only when<br>
> > > > > > > > > > > > > there's a valid implicit conversion like the void*<br>
> > > > > example?)<br>
> > > > > > > then we<br>
> > > > > > > > > > > > should<br>
> > > > > > > > > > > > > probably drop the top level const, "const char<br>
> > *const" -><br>
> > > > > > > "char*" -<br>
> > > > > > > > > > the<br>
> > > > > > > > > > > > top<br>
> > > > > > > > > > > > > level const on the first type is<br>
> > confusing/misleading, it's<br>
> > > > > > > only<br>
> > > > > > > > > > relevant<br>
> > > > > > > > > > > > > to show "const char*" and "char*".<br>
> > > > > > > > > > > > ><br>
> > > > > > > > > > > > ><br>
> > > > > > > > > > > > > On Wed, Nov 12, 2014 at 10:41 AM, Roman Divacky <<br>
> > > > > > > <a href="mailto:rdivacky@vlakno.cz">rdivacky@vlakno.cz</a>><br>
> > > > > > > > > > > > wrote:<br>
> > > > > > > > > > > > ><br>
> > > > > > > > > > > > > > I expanded the testcase and fixed the grammar in<br>
> > the<br>
> > > > > actual<br>
> > > > > > > > > > warning.<br>
> > > > > > > > > > > > > ><br>
> > > > > > > > > > > > > > New patch attached.<br>
> > > > > > > > > > > > > ><br>
> > > > > > > > > > > > > > On Tue, Nov 11, 2014 at 05:03:33PM -0800, David<br>
> > Blaikie<br>
> > > > > > > wrote:<br>
> > > > > > > > > > > > > > > (it's a bit easier if you include the test in<br>
> > the same<br>
> > > > > > > patch<br>
> > > > > > > > > > file -<br>
> > > > > > > > > > > > also<br>
> > > > > > > > > > > > > > > you can use Phabricator if you like - some<br>
> > reviewers<br>
> > > > > > > perefer it)<br>
> > > > > > > > > > > > > > ><br>
> > > > > > > > > > > > > > > Since you've got the loop there for seeing<br>
> > through<br>
> > > > > multiple<br>
> > > > > > > > > > levels of<br>
> > > > > > > > > > > > > > > pointer, should you have a test case that<br>
> > exercises<br>
> > > > > that<br>
> > > > > > > on a > 1<br>
> > > > > > > > > > > > level<br>
> > > > > > > > > > > > > > of<br>
> > > > > > > > > > > > > > > depth? Demonstrate that we warn on both levels<br>
> > (if<br>
> > > > > that's<br>
> > > > > > > the<br>
> > > > > > > > > > right<br>
> > > > > > > > > > > > thing<br>
> > > > > > > > > > > > > > > to do?)?<br>
> > > > > > > > > > > > > > ><br>
> > > > > > > > > > > > > > > Optionally (probably in a separate follow-up<br>
> > patch) you<br>
> > > > > > > could<br>
> > > > > > > > > > add a<br>
> > > > > > > > > > > > note<br>
> > > > > > > > > > > > > > > with a fixit to include the missing consts.<br>
> > > > > > > > > > > > > > ><br>
> > > > > > > > > > > > > > > On Tue, Nov 11, 2014 at 10:58 AM, Roman Divacky <<br>
> > > > > > > > > > <a href="mailto:rdivacky@vlakno.cz">rdivacky@vlakno.cz</a>><br>
> > > > > > > > > > > > > > wrote:<br>
> > > > > > > > > > > > > > ><br>
> > > > > > > > > > > > > > > > Hi,<br>
> > > > > > > > > > > > > > > ><br>
> > > > > > > > > > > > > > > > I implemented -Wcast-qual. The patch is<br>
> > actually<br>
> > > > > quite<br>
> > > > > > > short<br>
> > > > > > > > > > > > (attached<br>
> > > > > > > > > > > > > > + a<br>
> > > > > > > > > > > > > > > > test<br>
> > > > > > > > > > > > > > > > case).<br>
> > > > > > > > > > > > > > > ><br>
> > > > > > > > > > > > > > > > This fixes #13772 and also note that<br>
> > -Wcast-qual is<br>
> > > > > used<br>
> > > > > > > in<br>
> > > > > > > > > > llvm<br>
> > > > > > > > > > > > build<br>
> > > > > > > > > > > > > > > > itself.<br>
> > > > > > > > > > > > > > > ><br>
> > > > > > > > > > > > > > > > Is this ok to be commited? Thanks<br>
> > > > > > > > > > > > > > > ><br>
> > > > > > > > > > > > > > > > Roman<br>
> > > > > > > > > > > > > > > ><br>
> > > > > > > > > > > > > > > ><br>
> > > > > > > > > > > > > > > > _______________________________________________<br>
> > > > > > > > > > > > > > > > cfe-commits mailing list<br>
> > > > > > > > > > > > > > > > <a href="mailto:cfe-commits@cs.uiuc.edu">cfe-commits@cs.uiuc.edu</a><br>
> > > > > > > > > > > > > > > ><br>
> > > > > <a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
> > > > > > > > > > > > > > > ><br>
> > > > > > > > > > > > > > > ><br>
> > > > > > > > > > > > > ><br>
> > > > > > > > > > > ><br>
> > > > > > > > > ><br>
> > > > > > ><br>
> > > > > > > > Index: include/clang/Basic/DiagnosticGroups.td<br>
> > > > > > > ><br>
> > ===================================================================<br>
> > > > > > > > --- include/clang/Basic/DiagnosticGroups.td (revision 222228)<br>
> > > > > > > > +++ include/clang/Basic/DiagnosticGroups.td (working copy)<br>
> > > > > > > > @@ -60,7 +60,7 @@<br>
> > > > > > > > def KeywordCompat : DiagGroup<"keyword-compat">;<br>
> > > > > > > > def GNUCaseRange : DiagGroup<"gnu-case-range">;<br>
> > > > > > > > def CastAlign : DiagGroup<"cast-align">;<br>
> > > > > > > > -def : DiagGroup<"cast-qual">;<br>
> > > > > > > > +def CastQual : DiagGroup<"cast-qual">;<br>
> > > > > > > > def : DiagGroup<"char-align">;<br>
> > > > > > > > def Comment : DiagGroup<"comment">;<br>
> > > > > > > > def GNUComplexInteger : DiagGroup<"gnu-complex-integer">;<br>
> > > > > > > > Index: include/clang/Basic/DiagnosticSemaKinds.td<br>
> > > > > > > ><br>
> > ===================================================================<br>
> > > > > > > > --- include/clang/Basic/DiagnosticSemaKinds.td (revision<br>
> > > > > 222228)<br>
> > > > > > > > +++ include/clang/Basic/DiagnosticSemaKinds.td (working<br>
> > copy)<br>
> > > > > > > > @@ -6104,6 +6104,8 @@<br>
> > > > > > > > def warn_zero_size_struct_union_in_extern_c :<br>
> > > > > Warning<"%select{|empty<br>
> > > > > > > }0"<br>
> > > > > > > > "%select{struct|union}1 has size 0 in C, %select{size<br>
> > 1|non-zero<br>
> > > > > > > size}2 in C++">,<br>
> > > > > > > > InGroup<ExternCCompat>;<br>
> > > > > > > > +def warn_cast_qual : Warning<"cast from %0 to %1 drops %2<br>
> > > > > qualifier%3">,<br>
> > > > > > > > + InGroup<CastQual>, DefaultIgnore;<br>
> > > > > > > > } // End of general sema category.<br>
> > > > > > > ><br>
> > > > > > > > // inline asm.<br>
> > > > > > > > Index: lib/Sema/SemaCast.cpp<br>
> > > > > > > ><br>
> > ===================================================================<br>
> > > > > > > > --- lib/Sema/SemaCast.cpp (revision 222228)<br>
> > > > > > > > +++ lib/Sema/SemaCast.cpp (working copy)<br>
> > > > > > > > @@ -143,7 +143,10 @@<br>
> > > > > > > > }<br>
> > > > > > > ><br>
> > > > > > > > static bool CastsAwayConstness(Sema &Self, QualType SrcType,<br>
> > > > > QualType<br>
> > > > > > > DestType,<br>
> > > > > > > > - bool CheckCVR, bool<br>
> > > > > CheckObjCLifetime);<br>
> > > > > > > > + bool CheckCVR, bool<br>
> > > > > CheckObjCLifetime,<br>
> > > > > > > > + QualType *TheOffendingSrcType =<br>
> > > > > nullptr,<br>
> > > > > > > > + QualType *TheOffendingDestType<br>
> > =<br>
> > > > > nullptr,<br>
> > > > > > > > + Qualifiers *CastAwayQualifiers<br>
> > =<br>
> > > > > > > nullptr);<br>
> > > > > > > ><br>
> > > > > > > > // The Try functions attempt a specific way of casting. If<br>
> > they<br>
> > > > > > > succeed, they<br>
> > > > > > > > // return TC_Success. If their way of casting is not<br>
> > appropriate for<br>
> > > > > > > the given<br>
> > > > > > > > @@ -462,7 +465,10 @@<br>
> > > > > > > > /// \param CheckObjCLifetime Whether to check Objective-C<br>
> > lifetime<br>
> > > > > > > qualifiers.<br>
> > > > > > > > static bool<br>
> > > > > > > > CastsAwayConstness(Sema &Self, QualType SrcType, QualType<br>
> > DestType,<br>
> > > > > > > > - bool CheckCVR, bool CheckObjCLifetime) {<br>
> > > > > > > > + bool CheckCVR, bool CheckObjCLifetime,<br>
> > > > > > > > + QualType *TheOffendingSrcType,<br>
> > > > > > > > + QualType *TheOffendingDestType,<br>
> > > > > > > > + Qualifiers *CastAwayQualifiers) {<br>
> > > > > > > > // If the only checking we care about is for Objective-C<br>
> > lifetime<br>
> > > > > > > qualifiers,<br>
> > > > > > > > // and we're not in ARC mode, there's nothing to check.<br>
> > > > > > > > if (!CheckCVR && CheckObjCLifetime &&<br>
> > > > > > > > @@ -487,6 +493,8 @@<br>
> > > > > > > > // Find the qualifiers. We only care about cvr-qualifiers<br>
> > for the<br>
> > > > > > > > // purpose of this check, because other qualifiers (address<br>
> > > > > spaces,<br>
> > > > > > > > // Objective-C GC, etc.) are part of the type's identity.<br>
> > > > > > > > + QualType PrevUnwrappedSrcType = UnwrappedSrcType;<br>
> > > > > > > > + QualType PrevUnwrappedDestType = UnwrappedDestType;<br>
> > > > > > > > while (UnwrapDissimilarPointerTypes(UnwrappedSrcType,<br>
> > > > > > > UnwrappedDestType)) {<br>
> > > > > > > > // Determine the relevant qualifiers at this level.<br>
> > > > > > > > Qualifiers SrcQuals, DestQuals;<br>
> > > > > > > > @@ -497,6 +505,13 @@<br>
> > > > > > > > if (CheckCVR) {<br>
> > > > > > > ><br>
> > > > > RetainedSrcQuals.setCVRQualifiers(SrcQuals.getCVRQualifiers());<br>
> > > > > > > ><br>
> > > > > RetainedDestQuals.setCVRQualifiers(DestQuals.getCVRQualifiers());<br>
> > > > > > > > +<br>
> > > > > > > > + if (RetainedSrcQuals != RetainedDestQuals &&<br>
> > > > > TheOffendingSrcType<br>
> > > > > > > &&<br>
> > > > > > > > + TheOffendingDestType && CastAwayQualifiers) {<br>
> > > > > > > > + *TheOffendingSrcType = PrevUnwrappedSrcType;<br>
> > > > > > > > + *TheOffendingDestType = PrevUnwrappedDestType;<br>
> > > > > > > > + *CastAwayQualifiers = RetainedSrcQuals -<br>
> > RetainedDestQuals;<br>
> > > > > > > > + }<br>
> > > > > > > > }<br>
> > > > > > > ><br>
> > > > > > > > if (CheckObjCLifetime &&<br>
> > > > > > > > @@ -505,6 +520,9 @@<br>
> > > > > > > ><br>
> > > > > > > > cv1.push_back(RetainedSrcQuals);<br>
> > > > > > > > cv2.push_back(RetainedDestQuals);<br>
> > > > > > > > +<br>
> > > > > > > > + PrevUnwrappedSrcType = UnwrappedSrcType;<br>
> > > > > > > > + PrevUnwrappedDestType = UnwrappedDestType;<br>
> > > > > > > > }<br>
> > > > > > > > if (cv1.empty())<br>
> > > > > > > > return false;<br>
> > > > > > > > @@ -2371,6 +2389,28 @@<br>
> > > > > > > ><br>
> > > > > > > > if (Kind == CK_BitCast)<br>
> > > > > > > > checkCastAlign();<br>
> > > > > > > > +<br>
> > > > > > > > + // -Wcast-qual<br>
> > > > > > > > + QualType TheOffendingSrcType, TheOffendingDestType;<br>
> > > > > > > > + Qualifiers CastAwayQualifiers;<br>
> > > > > > > > + if (CastsAwayConstness(Self, SrcType, DestType, true, false,<br>
> > > > > > > > + &TheOffendingSrcType,<br>
> > &TheOffendingDestType,<br>
> > > > > > > > + &CastAwayQualifiers)) {<br>
> > > > > > > > + const char *qualifiers;<br>
> > > > > > > > + const char *suffix = "";<br>
> > > > > > > > + if (CastAwayQualifiers.hasConst() &&<br>
> > > > > > > CastAwayQualifiers.hasVolatile()) {<br>
> > > > > > > > + qualifiers = "const and volatile";<br>
> > > > > > > > + suffix = "s";<br>
> > > > > > > > + } else if (CastAwayQualifiers.hasConst())<br>
> > > > > > > > + qualifiers = "const";<br>
> > > > > > > > + else if (CastAwayQualifiers.hasVolatile())<br>
> > > > > > > > + qualifiers = "volatile";<br>
> > > > > > > > + else {<br>
> > > > > > > > + llvm_unreachable("Impossible qualifier");<br>
> > > > > > > > + }<br>
> > > > > > > > + Self.Diag(SrcExpr.get()->getLocStart(),<br>
> > diag::warn_cast_qual) <<<br>
> > > > > > > > + TheOffendingSrcType << TheOffendingDestType <<<br>
> > qualifiers <<<br>
> > > > > > > suffix;<br>
> > > > > > > > + }<br>
> > > > > > > > }<br>
> > > > > > > ><br>
> > > > > > > > ExprResult Sema::BuildCStyleCastExpr(SourceLocation LPLoc,<br>
> > > > > > > > Index: test/Sema/warn-cast-qual.c<br>
> > > > > > > ><br>
> > ===================================================================<br>
> > > > > > > > --- test/Sema/warn-cast-qual.c (revision 0)<br>
> > > > > > > > +++ test/Sema/warn-cast-qual.c (working copy)<br>
> > > > > > > > @@ -0,0 +1,19 @@<br>
> > > > > > > > +// RUN: %clang_cc1 -triple x86_64-unknown-unknown<br>
> > -fsyntax-only<br>
> > > > > > > -Wcast-qual -verify %s<br>
> > > > > > > > +<br>
> > > > > > > > +#include <stdint.h><br>
> > > > > > > > +<br>
> > > > > > > > +void foo() {<br>
> > > > > > > > + const char * const ptr = 0;<br>
> > > > > > > > + const char * const *ptrptr = 0;<br>
> > > > > > > > + char *y = (char *)ptr; // expected-warning {{cast from<br>
> > 'const<br>
> > > > > > > char *' to 'char *' drops const qualifier}}<br>
> > > > > > > > + char **y1 = (char **)ptrptr; // expected-warning<br>
> > {{cast from<br>
> > > > > > > 'const char *const' to 'char *' drops const qualifier}}<br>
> > > > > > > > + const char **y2 = (const char **)ptrptr; //<br>
> > expected-warning<br>
> > > > > {{cast<br>
> > > > > > > from 'const char *const *' to 'const char **' drops const<br>
> > qualifier}}<br>
> > > > > > > > +<br>
> > > > > > > > + char *z = (char *)(uintptr_t)(const void *)ptr; // no<br>
> > warning<br>
> > > > > > > > + char *z1 = (char *)(const void *)ptr; //<br>
> > expected-warning<br>
> > > > > {{cast<br>
> > > > > > > from 'const void *' to 'char *' drops const qualifier}}<br>
> > > > > > > > +<br>
> > > > > > > > + volatile char *vol = 0;<br>
> > > > > > > > + char *vol2 = (char *)vol; // expected-warning {{cast from<br>
> > > > > 'volatile<br>
> > > > > > > char *' to 'char *' drops volatile qualifier}}<br>
> > > > > > > > + const volatile char *volc = 0;<br>
> > > > > > > > + char *volc2 = (char *)volc; // expected-warning {{cast from<br>
> > 'const<br>
> > > > > > > volatile char *' to 'char *' drops const and volatile<br>
> > qualifiers}}<br>
> > > > > > > > +}<br>
> > > > > > ><br>
> > > > > > ><br>
> > > > ><br>
> ><br>
> > > Index: include/clang/Basic/DiagnosticGroups.td<br>
> > > ===================================================================<br>
> > > --- include/clang/Basic/DiagnosticGroups.td (revision 222421)<br>
> > > +++ include/clang/Basic/DiagnosticGroups.td (working copy)<br>
> > > @@ -60,7 +60,7 @@<br>
> > > def KeywordCompat : DiagGroup<"keyword-compat">;<br>
> > > def GNUCaseRange : DiagGroup<"gnu-case-range">;<br>
> > > def CastAlign : DiagGroup<"cast-align">;<br>
> > > -def : DiagGroup<"cast-qual">;<br>
> > > +def CastQual : DiagGroup<"cast-qual">;<br>
> > > def : DiagGroup<"char-align">;<br>
> > > def Comment : DiagGroup<"comment">;<br>
> > > def GNUComplexInteger : DiagGroup<"gnu-complex-integer">;<br>
> > > Index: include/clang/Basic/DiagnosticSemaKinds.td<br>
> > > ===================================================================<br>
> > > --- include/clang/Basic/DiagnosticSemaKinds.td (revision 222421)<br>
> > > +++ include/clang/Basic/DiagnosticSemaKinds.td (working copy)<br>
> > > @@ -6112,6 +6112,11 @@<br>
> > > def warn_zero_size_struct_union_in_extern_c : Warning<"%select{|empty<br>
> > }0"<br>
> > > "%select{struct|union}1 has size 0 in C, %select{size 1|non-zero<br>
> > size}2 in C++">,<br>
> > > InGroup<ExternCCompat>;<br>
> > > +def warn_cast_qual : Warning<"cast from %0 to %1 drops %select{const<br>
> > and"<br>
> > > + "volatile qualifiers|const qualifier|volatile qualifier}2">,<br>
> > > + InGroup<CastQual>, DefaultIgnore;<br>
> > > +def warn_cast_qual2 : Warning<"cast from %0 to %1 must have all<br>
> > intermediate "<br>
> > > + "pointers const qualified to be safe">, InGroup<CastQual>,<br>
> > DefaultIgnore;<br>
> > > } // End of general sema category.<br>
> > ><br>
> > > // inline asm.<br>
> > > Index: lib/Sema/SemaCast.cpp<br>
> > > ===================================================================<br>
> > > --- lib/Sema/SemaCast.cpp (revision 222421)<br>
> > > +++ lib/Sema/SemaCast.cpp (working copy)<br>
> > > @@ -142,9 +142,6 @@<br>
> > > };<br>
> > > }<br>
> > ><br>
> > > -static bool CastsAwayConstness(Sema &Self, QualType SrcType, QualType<br>
> > DestType,<br>
> > > - bool CheckCVR, bool CheckObjCLifetime);<br>
> > > -<br>
> > > // The Try functions attempt a specific way of casting. If they<br>
> > succeed, they<br>
> > > // return TC_Success. If their way of casting is not appropriate for<br>
> > the given<br>
> > > // arguments, they return TC_NotApplicable and *may* set diag to a<br>
> > diagnostic<br>
> > > @@ -462,7 +459,10 @@<br>
> > > /// \param CheckObjCLifetime Whether to check Objective-C lifetime<br>
> > qualifiers.<br>
> > > static bool<br>
> > > CastsAwayConstness(Sema &Self, QualType SrcType, QualType DestType,<br>
> > > - bool CheckCVR, bool CheckObjCLifetime) {<br>
> > > + bool CheckCVR, bool CheckObjCLifetime,<br>
> > > + QualType *TheOffendingSrcType = nullptr,<br>
> > > + QualType *TheOffendingDestType = nullptr,<br>
> > > + Qualifiers *CastAwayQualifiers = nullptr) {<br>
> > > // If the only checking we care about is for Objective-C lifetime<br>
> > qualifiers,<br>
> > > // and we're not in ARC mode, there's nothing to check.<br>
> > > if (!CheckCVR && CheckObjCLifetime &&<br>
> > > @@ -487,6 +487,8 @@<br>
> > > // Find the qualifiers. We only care about cvr-qualifiers for the<br>
> > > // purpose of this check, because other qualifiers (address spaces,<br>
> > > // Objective-C GC, etc.) are part of the type's identity.<br>
> > > + QualType PrevUnwrappedSrcType = UnwrappedSrcType;<br>
> > > + QualType PrevUnwrappedDestType = UnwrappedDestType;<br>
> > > while (UnwrapDissimilarPointerTypes(UnwrappedSrcType,<br>
> > UnwrappedDestType)) {<br>
> > > // Determine the relevant qualifiers at this level.<br>
> > > Qualifiers SrcQuals, DestQuals;<br>
> > > @@ -497,6 +499,13 @@<br>
> > > if (CheckCVR) {<br>
> > > RetainedSrcQuals.setCVRQualifiers(SrcQuals.getCVRQualifiers());<br>
> > > RetainedDestQuals.setCVRQualifiers(DestQuals.getCVRQualifiers());<br>
> > > +<br>
> > > + if (RetainedSrcQuals != RetainedDestQuals && TheOffendingSrcType<br>
> > &&<br>
> > > + TheOffendingDestType && CastAwayQualifiers) {<br>
> > > + *TheOffendingSrcType = PrevUnwrappedSrcType;<br>
> > > + *TheOffendingDestType = PrevUnwrappedDestType;<br>
> > > + *CastAwayQualifiers = RetainedSrcQuals - RetainedDestQuals;<br>
> > > + }<br>
> > > }<br>
> > ><br>
> > > if (CheckObjCLifetime &&<br>
> > > @@ -505,6 +514,9 @@<br>
> > ><br>
> > > cv1.push_back(RetainedSrcQuals);<br>
> > > cv2.push_back(RetainedDestQuals);<br>
> > > +<br>
> > > + PrevUnwrappedSrcType = UnwrappedSrcType;<br>
> > > + PrevUnwrappedDestType = UnwrappedDestType;<br>
> > > }<br>
> > > if (cv1.empty())<br>
> > > return false;<br>
> > > @@ -2371,6 +2383,29 @@<br>
> > ><br>
> > > if (Kind == CK_BitCast)<br>
> > > checkCastAlign();<br>
> > > +<br>
> > > + // -Wcast-qual<br>
> > > + QualType TheOffendingSrcType, TheOffendingDestType;<br>
> > > + Qualifiers CastAwayQualifiers;<br>
> > > + if (CastsAwayConstness(Self, SrcType, DestType, true, false,<br>
> > > + &TheOffendingSrcType, &TheOffendingDestType,<br>
> > > + &CastAwayQualifiers)) {<br>
> > > + int qualifiers = 0;<br>
> > > + if (CastAwayQualifiers.hasConst() &&<br>
> > CastAwayQualifiers.hasVolatile()) {<br>
> > > + qualifiers = 0;<br>
> > > + } else if (CastAwayQualifiers.hasConst()) {<br>
> > > + qualifiers = 1;<br>
> > > + } else if (CastAwayQualifiers.hasVolatile()) {<br>
> > > + qualifiers = 2;<br>
> > > + } else {<br>
> > > + // This is a variant of int **x; const int **y = (const int **)x;<br>
> > > + Self.Diag(SrcExpr.get()->getLocStart(), diag::warn_cast_qual2) <<<br>
> > > + SrcType << DestType;<br>
> > > + return;<br>
> > > + }<br>
> > > + Self.Diag(SrcExpr.get()->getLocStart(), diag::warn_cast_qual) <<<br>
> > > + TheOffendingSrcType << TheOffendingDestType << qualifiers;<br>
> > > + }<br>
> > > }<br>
> > ><br>
> > > ExprResult Sema::BuildCStyleCastExpr(SourceLocation LPLoc,<br>
> > > Index: test/Sema/warn-cast-qual.c<br>
> > > ===================================================================<br>
> > > --- test/Sema/warn-cast-qual.c (revision 0)<br>
> > > +++ test/Sema/warn-cast-qual.c (working copy)<br>
> > > @@ -0,0 +1,29 @@<br>
> > > +// RUN: %clang_cc1 -triple x86_64-unknown-unknown -fsyntax-only<br>
> > -Wcast-qual -verify %s<br>
> > > +<br>
> > > +#include <stdint.h><br>
> > > +<br>
> > > +void foo() {<br>
> > > + const char * const ptr = 0;<br>
> > > + const char * const *ptrptr = 0;<br>
> > > + char *y = (char *)ptr; // expected-warning {{cast from 'const<br>
> > char *' to 'char *' drops const qualifier}}<br>
> > > + char **y1 = (char **)ptrptr; // expected-warning {{cast from<br>
> > 'const char *const' to 'char *' drops const qualifier}}<br>
> > > + const char **y2 = (const char **)ptrptr; // expected-warning {{cast<br>
> > from 'const char *const *' to 'const char **' drops const qualifier}}<br>
> > > +<br>
> > > + char *z = (char *)(uintptr_t)(const void *)ptr; // no warning<br>
> > > + char *z1 = (char *)(const void *)ptr; // expected-warning {{cast<br>
> > from 'const void *' to 'char *' drops const qualifier}}<br>
> > > +<br>
> > > + volatile char *vol = 0;<br>
> > > + char *vol2 = (char *)vol; // expected-warning {{cast from 'volatile<br>
> > char *' to 'char *' drops volatile qualifier}}<br>
> > > + const volatile char *volc = 0;<br>
> > > + char *volc2 = (char *)volc; // expected-warning {{cast from 'const<br>
> > volatile char *' to 'char *' drops const and volatile qualifiers}}<br>
> > > +<br>
> > > + int **intptrptr;<br>
> > > + const int **intptrptrc = (const int **)intptrptr; // expected-warning<br>
> > {{cast from 'int **' to 'const int **' must have all intermediate pointers<br>
> > const qualified}}<br>
> > > + volatile int **intptrptrv = (volatile int **)intptrptr; //<br>
> > expected-warning {{cast from 'int **' to 'volatile int **' must have all<br>
> > intermediate pointers const qualified}}<br>
> > > +<br>
> > > + int *intptr;<br>
> > > + const int *intptrc = (const int *)intptr; // no warning<br>
> > > +<br>
> > > + const char **charptrptrc;<br>
> > > + char **charptrptr = (char **)charptrptrc; // expected-warning {{cast<br>
> > from 'const char *' to 'char *' drops const qualifier}}<br>
> > > +}<br>
> ><br>
> ><br>
</div></div></blockquote></div><br></div></div>