On 01/31/2012 07:58 AM, Philipp Hahn wrote:
> Hello Eric,
>
> Am Montag 30 Januar 2012 20:18:15 schrieb Eric Blake:
>>> + if (diff > 0) {
>>> + tmp = realloc((void *)*buf, new_len);
>>
>> We should not be using realloc in this file, but should be using
>> VIR_RESIZE_N or similar.
>
> Okay, makes the code even more readable.
>
>>> + memmove(token_start, replacement, replacement_len);
>>
>> But this would be more efficient as memcpy, since replacement (better
>> not) overlap with token_start.
>
> I know of some projects which forbid memcpy() because of the missing overlap
> handling. But if this is okay with libvirt, I'll use it.
memcpy() is safe when the code is provably visiting non-overlapping
strings, as is the case with your second use of memmove(). memcpy() is
a mistake where the code is provably visiting overlapping strings, as is
the case with your first memmove(). On some libc, memmove and memcpy
are the same function; but since the standards allow memcpy to be more
efficient than memmove by exploiting the non-overlap guarantee, you
might as well use the more efficient method in the cases where things
really are non-overlapping. Libvirt isn't so strict as to pessimize
correct uses of memcpy() just because it can sometime be misused. It
also helps that static analyzers like Coverity are getting pretty good
at pointing out mis-uses of memcpy().
> I hope performance will never be a problem with this simple test scenario,
> because then doing one realloc() instead of one for each found would be
> better.
You do have a valid counterpoint here - this is a test, and not a hot
path, so the savings of memcpy() vs. memmove() are in the noise compared
to any extra realloc(); thankfully, neither overhead is worth worrying
about too much. At any rate, I'll review your v2 now.
--
Eric Blake eblake@xxxxxxxxxx +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list