Stephen Gallagher wrote:
On 08/28/2009 05:40 AM, Jakub Hrozek wrote:
>> Thank you for the review.
>> My comments are inline.
>>> Couple of comments:
>>> * Instead of putting libprovider.la into _SOURCES, maybe you wanted to
>>> add it to libelapi_la_LIBADD?
>>>
>> I consulted Steve on this one and showed him the makefile.
>> He was Ok with it. But may be you right.
> Then it's probably right, it's just something that I noticed.
While I sincerely appreciate the vote of confidence, I made a mistake.
You are correct that is should be part of _LIBADD, not _SOURCES here.
Well next
patch will correct it.
I already have it the code
>> I will wait for Steven to take a second look at this one.
>>> * in elapi_print_sink_ctx() instead of casting pointer to uint32_t and
>>> printing with %X, you should use %p without casting the pointer
>>>
>> Most of my C is in pre C99 style. I just did not know about the
option.
>> I will definitely switch it to %p if it is portable between platforms.
>> Are you sure it is portable?
> Yes, it is part of C standard.
>>> * why are elapi_sink_cfg.required and elapi_sink_cfg.synch defined as
>>> uint32_t? They seem to have bool semantics, maybe bool would be more
>>> appropriate. Anyhow, using uint32_t to store unsigned int (or unsigned
>>> char in case of get_bool_config_value() even though it should not
matter
>>> here as unsigned char will be smaller than 32 bits) return value might
>>> not be correct on all platforms.
>> Here is the explanation.
>> I started with "bool".
>> What happened is that when bool is a part of structure it gets aligned
>> on the boundary
>> of the word.
>> In case of 32bit system on the boundary of the 4 bytes.
>> The tree bytes in this case remain uninitialized and valgrind screams
>> and shouts about them.
>> So I decided that since the memory is wasted anyways use 4 bytes and
>> make valgrind happy.
>> This is why I switched from bools to uint32_t.
In order to waste less memory, wouldn't a uint8_t be a better choice?
It is the
structure that would be allocated once per instance of ELAPI.
I do not see it being a big deal. And the compiler would try to align
things in the struct anyways.
I will keep it in mind in cases where memory is allocated frequently and
large volumes.
[snip]
memset() is a bad operation to use when dealing with
strings/null-terminated arrays. It's perfectly valid to use on structs
(and advisable, even). The reason it's bad for strings is that it does
two things unnecessarily: 1) it modifies every entry in the array when
it needs only set the first entry to null-termination and 2) it gives a
false sense of security that the string is now guaranteed to be
terminated (which is not the case if you write past the end of the
buffer). On the other hand, its use in a struct is completely reasonable
because (unless there are static buffers in the struct) you are only
writing a minimal set of zeroes.
I will check with valgrind if my observation about memset and calloc is
false I will
revert to those where it makes sense.
I think it makes sense to wait for the next patch and review the second
one first and then push them together.
Thanks,
Dmitri