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 > > > > > > > > > > > > > >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
