Jeff King <peff@xxxxxxxx> writes:
> On Sat, Mar 15, 2014 at 05:05:29PM +0100, Thomas Rast wrote:
>
>> > diff --git a/builtin/mv.c b/builtin/mv.c
>> > index f99c91e..b20cd95 100644
>> > --- a/builtin/mv.c
>> > +++ b/builtin/mv.c
>> > @@ -230,6 +230,11 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
>> > memmove(destination + i,
>> > destination + i + 1,
>> > (argc - i) * sizeof(char *));
>> > + memmove(modes + i, modes + i + 1,
>> > + (argc - i) * sizeof(char *));
>>
>> This isn't right -- you are computing the size of things to be moved
>> based on a type of char*, but 'modes' is an enum.
>>
>> (Valgrind spotted this.)
>
> Maybe using sizeof(*destination) and sizeof(*modes) would make this less
> error-prone?
>
> -Peff
Would it make sense to go one step further to introduce two macros
to make this kind of screw-up less likely?
1. "array" is an array that holds "nr" elements. Move "count"
elements starting at index "at" down to remove them.
#define MOVE_DOWN(array, nr, at, count)
The implementation should take advantage of sizeof(*array) to
come up with the number of bytes to move.
2. "array" is an array that holds "nr" elements. Move "count"
elements starting at index "at" up to make room to copy new
elements in.
#define MOVE_UP(array, nr, at, count)
The implementation should take advantage of sizeof(*array) to
come up with the number of bytes to move.
Optionally, to make 2. even safer, these macros could take "alloc"
to say that "array" has memory allocated to hold "alloc" elements,
and the implementation may check "nr + count" does not overflow
"alloc". This would make 1. and 2. asymmetric (move-down can do no
validation using "alloc", but move-up would be helped), so I am not
sure it is a good idea.
After letting my eyes coast over hits from "git grep memmove", there
do seem to be some places that these would help readability, but not
very many.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html