Hello Sumit, thanks for comments and sorry for my delayed response, I'm addressing the
issues right now, in this mail I just want quickly discuss the concern you have about
in loop declaration.
I generally I prefer this kind of declaration as it limits scope of variables to minimum
and thus makes it easier to read and comprehend (It makes it obvious that variable is
used just in this single loop and it's insignificant for the rest of the code).
In my experience there's not a performance hit at all some even claim that by limiting
scope of variables you give hints to compiler and thus generated code is faster.
This topic was discussed for example here:
http://stackoverflow.com/questions/407255/difference-between-declaring-va...
http://stackoverflow.com/questions/1956353/efficiency-of-c-variable-decla...
I think that the conclusion of these threads complies with what I have stated above. But I
suppose it would be possible to find threads claiming opposite so I'm aware it's
not definite source of truth :-)
Anyway, If you prefer If I kept all declaration at the begging of the function for legacy
purposes or other I will do it (without any hard feelings).
Thanks for patience.
On 01/06/2016 04:34 PM, Sumit Bose wrote:
> ...
>
>> @@ -447,8 +437,13 @@ enum idmap_error_code sss_idmap_check_collision(struct
sss_idmap_ctx *ctx,
>> enum idmap_error_code err;
>>
>> for (dom = ctx->idmap_domain_info; dom != NULL; dom = dom->next) {
>> - err = sss_idmap_check_collision_ex(dom->name, dom->sid,
dom->range,
>> - dom->first_rid, dom->range_id,
>> + struct sss_idmap_range range = { dom->range_params.min_id,
>> + dom->range_params.max_id };
>
> In general I agree with the refactoring in this patch, but I don't like
> the declaration of variables in the code. Especially in the case here
> and below where variables are declared inside a loop. Mainly because I
> do not know if compilers are able handle this smart or if this might be
> more expensive than declaring the variables outside of the loop.
>
>> +
>> + err = sss_idmap_check_collision_ex(dom->name, dom->sid,
>> + &range,
>> + dom->range_params.first_rid,
>> + dom->range_params.range_id,
>> dom->external_mapping,
>> n_name, n_sid, n_range,
n_first_rid,
>> n_range_id, n_external_mapping);
>> @@ -467,12 +462,22 @@ static enum idmap_error_code dom_check_collision(
>> enum idmap_error_code err;
>>
>> for (dom = dom_list; dom != NULL; dom = dom->next) {
>> - err = sss_idmap_check_collision_ex(dom->name, dom->sid,
dom->range,
>> - dom->first_rid, dom->range_id,
>> + struct sss_idmap_range range = { dom->range_params.min_id,
>> + dom->range_params.max_id };
>> + struct sss_idmap_range new_dom_range = {
>> + new_dom->range_params.min_id,
>> + new_dom->range_params.max_id };
>> +
>> +
>> + err = sss_idmap_check_collision_ex(dom->name, dom->sid,
>> + &range,
>> + dom->range_params.first_rid,
>> + dom->range_params.range_id,
>> dom->external_mapping,
>> new_dom->name, new_dom->sid,
>> - new_dom->range,
new_dom->first_rid,
>> - new_dom->range_id,
>> + &new_dom_range,
>> + new_dom->range_params.first_rid,
>> + new_dom->range_params.range_id,
>> new_dom->external_mapping);
>> if (err != IDMAP_SUCCESS) {
>> return err;
>
> ...