-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
On 12/22/2009 03:03 PM, Dmitri Pal wrote:
>
>> I do not see why anything should be changed here.
>
> The problem is the use of "ret" in both the return from the function and
> as the memory location for the copied code. It's the same variable
> having data returned to it in two different ways from the same function
> call. This is dangerous. It should look like
>
> ret1 = *((char **)ref_array_get(ra2, i, &ret2));
> if (strcmp(ret1, ret2) != 0) {
> //FAIL
> }
>
>
Ah now I understand what you are talking about.
Yes I agree it is generally dangerous and should be discouraged but the
unit-test checks that even if it is done this way it is Ok.
I should probably just add a comment about it to discourage this kind of
use and explain that this is a test to validate that it will not crash
if it is done this way.
Fine, but please implement additionally the test I described above, as
it will prove that the two return values are equivalent.
>> When you have time to do this.
>> I have a bigger fish to fry.
>> I am not arguing that these changes would make sense.
>> I argue that these changes are not required for the first patch that
>> introduces the interface.
>> If can't be expected to be perfect from get go.
>> It should be good enough to be able to move on with main goals.
>> Just a difference in the approach.
>
> The whole purpose of a review is so that only stable code reaches the
> repository (hopefully).
>
IMO this is a flaw in the process.
Stable code is not a flaw :)
>
>> Hm? I do not buy it. If you want to test the validity of the object you
>> check if it is NULL rather then call a length function.
>> The whole point of such simplified inteface is that you check for the
>> object validity once at the top and then just get it length when you
>> need to.
>
>> something like
>
>> int myfun(ref_array *ra)
>> {
>> ...
>> if (!ra) {
>> ...
>> return EINVAL;
>> }
>
>> len = ref_array_len();
>
>> ....
>> }
>
>> Now why check for ra twice and add overhead of error handling to a very
>> simple function that returns a property of the object?
>> It is like a get method in C++. It just returns value.
>
> I understand what you're saying, but you're attempting to enforce a
> coding standard in a function prototype. You can't know how a consumer
> of the API is going to use this call, so you have to protect against it.
> A NULL check is a very cheap operation.
>
>
This is exactly what I do internally:
/* Get length */
uint32_t ref_array_len(struct ref_array *ra)
{
TRACE_FLOW_STRING("ref_array_len", "Entry");
if (!ra) {
TRACE_ERROR_STRING("Uninitialized argument.", "");
return 0;
}
TRACE_FLOW_STRING("ref_array_len", "Exit");
return ra->len;
}
The function is written this way for convenience so that I do not need
to check and handle the error (and this is where the cost is, not the
check itself).
This is the whole point and value of the call.
You just asking to make it less usable and force the caller to add more
code in the error handling for now reason.
IMO it is a wrong approach.
How about setting errno=EINVAL in the if(!ra) block? If the caller
cares, they can check its value if they get zero back.
I'm not a fan of errno, but this might fit your workflow better as you
describe it.
- --
Stephen Gallagher
RHCE 804006346421761
Delivering value year after year.
Red Hat ranks #1 in value among software vendors.
http://www.redhat.com/promo/vendor/
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)
Comment: Using GnuPG with Fedora -
http://enigmail.mozdev.org/
iEYEARECAAYFAksxMA4ACgkQeiVVYja6o6NXfwCbB3z0UE7K5FYIqDcGMoCKoJIY
+DwAn2MwEIxrto/qNd8vkAfOm4wI/QHY
=44zO
-----END PGP SIGNATURE-----