On Mon, Jan 11, 2016 at 01:03:33PM +0100, Pavel Reichl wrote:
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).
Thank you for the link, so it looks compilers can handle this well. I'm
fine with this scheme, iirc we had this discussion for the loop index
before and agreed that it makes sense to reduce the scope where possilbe
to the loop. But I don't remember if there was an agreement about
variabled inside the loop as well? But if we can agree on this I would
like to ask you to update the paragraph in the coding style which
currently reads "HIGHLY RECOMMENDED: Always declare all variables at the
top of the function, normally try to avoid declaring local variables in
internal loops.".
bye,
Sumit
>
> 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;
> >
> >...
> _______________________________________________
> sssd-devel mailing list
> sssd-devel(a)lists.fedorahosted.org
>
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org