<div dir="ltr">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?<br><br>Chatting to Richard over lunch he mentioned an interesting case where we might want to warn:<br><br> int **x;<div> auto y = (const int **)x;<br><br>which, if it were a static_cast, would warn:<br><br><div><font face="monospace"> cast.cpp:2:10: error: static_cast from 'int **' to 'const int **' is not allowed</font></div><div><font face="monospace"> auto y = static_cast<const int **>(x);</font></div><div><font face="monospace"> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~<br><br></font></div></div><div>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:<br><br>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.<br><br>Sorry to go through so many iterations - it didn't occur to me until Richard mentioned it that there was this more general approach.<br><br>(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)</div></div><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Nov 14, 2014 at 12:02 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">Like this?<br>
<div class="HOEnZb"><div class="h5"><br>
On Fri, Nov 14, 2014 at 11:20:49AM -0800, David Blaikie wrote:<br>
> I take it this is consistent with the GCC warning - in terms of warning on<br>
> the innermost issue, reporting const or volatile - what about 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 null, they'll be<br>
> written to on the first loop iteration as needed - avoiding excess<br>
> initialization helps tools like MSan find more bugs rather than the program<br>
> silently using unintended default values<br>
><br>
> * InnerMostDestType and InnerMostSrcType will be dangling pointers after<br>
> the while loop (so accessing them in the proceeding 'if' is UB)<br>
><br>
> * you don't need to check both InnerMostDestType and InnerMostSrcType in<br>
> the following if - it's invariant that if one is non-null (you can use<br>
> QualType values rather than QualType* to address the previous bug, and use<br>
> QualTypes "isNull()" member function here) so is the other<br>
><br>
> On Fri, Nov 14, 2014 at 11:07 AM, Roman Divacky <<a href="mailto:rdivacky@vlakno.cz">rdivacky@vlakno.cz</a>> wrote:<br>
><br>
> > Actually, try this patch. It includes check for volatile as well.<br>
> ><br>
> > On Wed, Nov 12, 2014 at 12:39:20PM -0800, David Blaikie wrote:<br>
> > > [+Richard for oversight]<br>
> > ><br>
> > > char **y1 = (char **)ptrptr; // expected-warning {{cast from 'const char<br>
> > > *const *' to 'char **' drops const qualifier}} expected-warning {{cast<br>
> > from<br>
> > > 'const char *const' to 'char *' drops const qualifier}}<br>
> > ><br>
> > > I think if we're going to warn on multiple layers (I'm not sure that's<br>
> > > ideal - is that consistent with GCC's warning? Does GCC warn on<br>
> > mismatched<br>
> > > types too - "const T*" -> "U*"? - do we warn there too, or only when<br>
> > > there's a valid implicit conversion like the void* example?) then we<br>
> > should<br>
> > > probably drop the top level const, "const char *const" -> "char*" - the<br>
> > top<br>
> > > level const on the first type is confusing/misleading, it's only relevant<br>
> > > to show "const char*" and "char*".<br>
> > ><br>
> > ><br>
> > > On Wed, Nov 12, 2014 at 10:41 AM, Roman Divacky <<a href="mailto:rdivacky@vlakno.cz">rdivacky@vlakno.cz</a>><br>
> > wrote:<br>
> > ><br>
> > > > I expanded the testcase and fixed the grammar in the actual warning.<br>
> > > ><br>
> > > > New patch attached.<br>
> > > ><br>
> > > > On Tue, Nov 11, 2014 at 05:03:33PM -0800, David Blaikie wrote:<br>
> > > > > (it's a bit easier if you include the test in the same patch file -<br>
> > also<br>
> > > > > you can use Phabricator if you like - some reviewers perefer it)<br>
> > > > ><br>
> > > > > Since you've got the loop there for seeing through multiple levels of<br>
> > > > > pointer, should you have a test case that exercises that on a > 1<br>
> > level<br>
> > > > of<br>
> > > > > depth? Demonstrate that we warn on both levels (if that's the right<br>
> > thing<br>
> > > > > to do?)?<br>
> > > > ><br>
> > > > > Optionally (probably in a separate follow-up patch) you could 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 <<a href="mailto:rdivacky@vlakno.cz">rdivacky@vlakno.cz</a>><br>
> > > > wrote:<br>
> > > > ><br>
> > > > > > Hi,<br>
> > > > > ><br>
> > > > > > I implemented -Wcast-qual. The patch is actually quite short<br>
> > (attached<br>
> > > > + a<br>
> > > > > > test<br>
> > > > > > case).<br>
> > > > > ><br>
> > > > > > This fixes #13772 and also note that -Wcast-qual is used in 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>
> > > > > > <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>
</div></div></blockquote></div><br></div>