Optimization of add_dt_to_dt_list() in resolve.c

[Andrew Benson]

Hi Janus,

On Wednesday, May 30, 2018 10:42:46 PM PDT Janus Weil wrote:
> Hi Andrew,
> 
> 2018-05-29 22:24 GMT+02:00 Andrew Benson <abenson@carnegiescience.edu>:
> > Yes - definitely possible to remove gfc_dt_list entirely - new patch is
> > attached.
> 
> since you already got some 'contentual' comments, I'll give you some
> more 'formal' ones ...
> 
> 
> +  if (!derived->dt_next) {
> +    if (gfc_derived_types) {
> +      derived->dt_next = gfc_derived_types->dt_next;
> +      gfc_derived_types->dt_next = derived;
> +    } else {
> +      derived->dt_next = derived;
> +    }
> +    gfc_derived_types = derived;
> +  }
>
> Here and in some other hunks you're not conforming with the GNU coding
> style, which demands that opening and closing braces should be on
> separate lines (and at a new indentation level).

Thanks - I'll fix those cases.
 
> -  for (dt = gfc_derived_types; dt; dt = dt->next)
> -    gfc_copy_dt_decls_ifequal (derived, dt->derived, false);
> -
> +  if (gfc_derived_types) {
> +    dt = gfc_derived_types;
> +    for (;;)
> +      {
> +    gfc_copy_dt_decls_ifequal (derived, dt, false);
> +    if (dt->dt_next == gfc_derived_types)
> +      break;
> +    dt = dt->dt_next;
> +      }
> +  }
> 
> Is there a particular reason why you changed the loop to "for (;;)" ?
> I find the old style a bit clearer and more compact. Also I think it's
> more common in gfortran.

I changed the original for loop since it wasn't possible (as far as I could 
see) to have an exit condition that worked now that list is circularly linked 
(i.e. "dt" never becomes NULL, and testing for dt->dt_next == 
gfc_derived_types doesn't work as it would miss the final entry in the list). 
So, I used for(;;) and added the exit condition explicitly into the loop.

But, I agree, it's not very clear. An alternative would be something such as:
 
-  for (dt = gfc_derived_types; dt; dt = dt->next)
-    gfc_copy_dt_decls_ifequal (derived, dt->derived, false);
-
+  for (dt = gfc_derived_types; dt; dt = dt->dt_next)
+    {
+      gfc_copy_dt_decls_ifequal (derived, dt, false);
+      if (dt->dt_next == gfc_derived_types)
+	break;
+    }
+  

which is more compact, and has the advantage that if doesn't require an "if 
(gfc_derived_types)". Do you think that's a better approach?

> Btw, do you already have a copyright assignment on file? If not,
> you'll probably need one (see https://gcc.gnu.org/contribute.html).

I don't, so will need to get that figured out.

> Thanks for your contribution and welcome to the gfortran team :)

Thanks!

-Andrew

-- 

* Andrew Benson: http://users.obs.carnegiescience.edu/abenson/contact.html

* Galacticus: https://bitbucket.org/abensonca/galacticus