[prev in list] [next in list] [prev in thread] [next in thread] 

List:       cfe-commits
Subject:    Re: [cfe-commits] r157722 - in /cfe/trunk: include/clang/Basic/DiagnosticSemaKinds.td lib/Sema/SemaC
From:       Anna Zaks <ganna () apple ! com>
Date:       2012-05-31 18:08:30
Message-ID: 4EB827F3-E711-45B8-AEC5-65A7F2D0CE40 () apple ! com
[Download RAW message or body]


On May 30, 2012, at 6:03 PM, Matt Beaumont-Gay wrote:

> On Wed, May 30, 2012 at 5:06 PM, Anna Zaks <ganna@apple.com> wrote:
> > 
> > On May 30, 2012, at 4:22 PM, Matt Beaumont-Gay wrote:
> > 
> > > On Wed, May 30, 2012 at 4:14 PM, Anna Zaks <ganna@apple.com> wrote:
> > > > Author: zaks
> > > > Date: Wed May 30 18:14:52 2012
> > > > New Revision: 157722
> > > > 
> > > > URL: http://llvm.org/viewvc/llvm-project?rev=157722&view=rev
> > > > Log:
> > > > Change wording of 'memcpy' type mismatch warning and remove fixit.
> > > > 
> > > > As per comments following r157659.
> > > > 
> > > > Removed:
> > > > cfe/trunk/test/SemaCXX/warn-memset-bad-sizeof-fixit.cpp
> > > > Modified:
> > > > cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
> > > > cfe/trunk/lib/Sema/SemaChecking.cpp
> > > > cfe/trunk/test/SemaCXX/warn-memset-bad-sizeof.cpp
> > > > 
> > > > Modified: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
> > > > URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td?rev=157722&r1=157721&r2=157722&view=diff
> > > >  ==============================================================================
> > > >                 
> > > > --- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td (original)
> > > > +++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td Wed May 30 18:14:52 \
> > > > 2012 @@ -335,10 +335,14 @@
> > > > def note_bad_memaccess_silence : Note<
> > > > "explicitly cast the pointer to silence this warning">;
> > > > def warn_sizeof_pointer_expr_memaccess : Warning<
> > > > -  "argument to 'sizeof' in '%0' call is the same expression as the "
> > > > -  "%select{destination|source}1; did you mean to "
> > > > -  "%select{dereference it|remove the addressof|provide an explicit \
> > > > length}2?">, +  "'%0' call operates on objects of type %1 while the size is \
> > > > based on a " +  "different type %2">,
> > > 
> > > I'm not thrilled with this new wording. These functions operate on
> > > bytes, not objects.
> > 
> > Is "values" better than "objects"?
> > 
> > 'memcpy' is usually used to copy over a number of values of the given type. \
> >                 Especially in the case the waring triggers (sizeof is used to \
> >                 compute the size):
> > - memcpy(to, from, sizeof(to));
> > + memcpy(to, from, sizeof(*to)*N);
> 
> I just looked over the cleanups I did when we turned this warning on
> in our codebase. Many of the buggy memcpy calls needed a count
> multiplier, but on the other hand, almost none of the buggy memset
> calls did.
> 
> In general, I don't think we need to be any more clever or explicit in
> terms of diagnostic wording here, nor do I think we need a fixit
> (which we can't get right with particularly high confidence). Just
> pointing out the problem is hugely valuable, and it doesn't feel like
> guessing at what the programmer meant is going to add a lot more
> value.
> 
Here is the change I made on an example:

-  warning: argument to 'sizeof' in 'memcpy' call is the same expression as the \
                destination; did you mean to dereference it?
-  memcpy(to, from, sizeof(to))
-                   ~~                      ~~
+  warning: 'memcpy' call operates on objects of type 'Blah' while the size is based \
on a different type 'Blah*'.  +  memcpy(to, from, sizeof(to))
+                   ~~                      ~~
+  note: did you mean to dereference the argument to 'sizeof' (and multiply it by the \
number of elements)?

The original wording was very specific about telling the user what the fix should be \
- it sounded very much like a fixit. (This is why I though that we should add a fixit \
here in the first place; see more explanation in r157659 thread.) I think the new \
note is better. The part about multiplying by the number of elements is in \
parentheses, which highlights that it's optional. I think it is quite feasible to \
intend multiplication in 'memset' as well as 'memcpy'. Would be interesting to see \
how many issues you found with this warning. We'll probably be able to find many more \
if we extend it to handle  "memcpy(to, form, sizeof(to)*N)", which should be quite \
easy to do.

The new wording of the warning is targeted to explaining the user what the logical \
issue with their code is. When I read "argument to 'sizeof' in 'memcpy' call is the \
same expression as the destination" I don't immediately understand what's going on \
without parsing the call expression.

Anna.

> -Matt

_______________________________________________
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits


[prev in list] [next in list] [prev in thread] [next in thread]