[cfe-commits] r157722 - in /cfe/trunk: include/clang/Basic/DiagnosticSemaKinds.td lib/Sema/SemaChecking.cpp test/SemaCXX/warn-memset-bad-sizeof-fixit.cpp test/SemaCXX/warn-memset-bad-sizeof.cpp

[Anna Zaks]

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

> On Wed, May 30, 2012 at 5:06 PM, Anna Zaks <ganna at 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 at 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