> 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.
> 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.
> 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.
_______________________________________________
sssd-devel mailing list
sssd-devel(a)lists.fedorahosted.org
https://fedorahosted.org/mailman/listinfo/sssd-devel
--
Thank you,
Dmitri Pal
Engineering Manager IPA project,
Red Hat Inc.
-------------------------------
Looking to carve out IT costs?
www.redhat.com/carveoutcosts/