[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

[Matt Beaumont-Gay]

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.

-Matt