On 04/26/2016 11:23 PM, Martin Sebor wrote:
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).
I've thought about some of these cases. The problem is there are
legitimate cases of calling memset for only part of an array. I wanted
to start with something that is unlikely to give false positives.
A multiplication by the wrong sizeof would be a nice thing to spot.
Would you like to work on followup patches? I probably won't get to it
in a while.
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.)
Probably a good idea.
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).
Fixed.
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
I'm finding this too verbose, but I guess that's a matter of taste.
Finally, I would be remiss not to mention that the patch has
an instance of trailing space in it (gasp! ;)
Fixed before committing.
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.
The first part is probably true, but we have code review exactly
_because_ people make mistakes. Without it, the code base would
degenerate rapidly. So, well spotted.
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!
I don't believe in tools for this, Machines are stupid, and I think the
problem is AI-complete. In this case the check-GNU-style script has two
other complaints about the patch which a human can tell are wrong.
Enforcing patches to pass a mechanical check would be a mistake IMO,
since it would force people into contortions trying to placate an
unintelligent program, making code quality worse.
Bernd
