On 04/25/2016 03:07 AM, Bernd Schmidt wrote:
On 04/22/2016 03:57 PM, Jason Merrill wrote:
This looks good, but can we move the code into c-common rather than
duplicate it?
That would be this patch. Also passes testing on x86_64-linux.
Hi Bernd,
I like the idea behind the patch! So much so in fact, and since
it happens to be in an area where I've been doing some of my own
work, I have a few observations and suggestions. Please forgive
me for the length of the email.
The documentation for the new option implies that it should warn
for calls to memset where the third argument contains the number
of elements not multiplied by the element size. But in my (quick)
testing it only warns when the argument is a constant equal to
the number of elements and less than the size of the array. For
example, neither of the following is diagnosed:
int a [4];
__builtin_memset (a, 0, 2 + 2);
__builtin_memset (a, 0, 4 * 1);
__builtin_memset (a, 0, 3);
__builtin_memset (a, 0, 4 * sizeof a);
If it's possible and not too difficult, it would be nice if
the detection logic could be made a bit smarter to also diagnose
these less trivial cases (and matched the documented behavior).
Even beyond that, I also wonder if the warning could also be
issued when writing any constant number of bytes that is not
a multiple of the element size. This would be useful not just
for memset but also for memcpy. (The premise being that it's
unusual to want to zero out or copy just a few bytes of any
array element and leave the remaining bytes of that element
unchanged.)
I also have a comment on the text and content of the warning:
memset used with length equal to number of elements without
multiplication with element size
FWIW, multiplication is typically done "by" a number (not with
one). More important, I often find it helpful if a diagnostic
spells out the numeric values involved in the computation (e.g.,
array bounds or sizes). I find this especially helpful in C++
where the values of symbolic constants are often the result of
non-trivial computations involving non-type template parameters
or constexpr variables or function calls, and thus difficult
to derive just by looking at the source code. Here's an idea
for rewording the diagnostic to include this information:
warning: memset called to set '3' bytes which is not a positive
multiple of element size in array 'a' with type int[3]'
note: array 'a' is declared here
Finally, I would be remiss not to mention that the patch has
an instance of trailing space in it (gasp! ;) Personally,
I'm not bothered by it but it seems like a good opportunity
to highlight that these things happen even to the most careful
of us, and not necessarily as a result of not being careful or
aware of the coding guidelines. My point is that no amount of
documentation will or diligence will prevent these kinds of
problems, and dwelling on them in code reviews isn't the best
use of our time. Let's put in place a tool that takes care of
these nits for us so we can focus on things a tool can't help
us with!
Martin
