Re: ABSTRACT interfaces + PROCEDURE declarations

[Tobias Schlüter <tobias dot schlueter at physik dot uni-muenchen dot de>]

Janus Weil wrote:
> Hi all,
> attached you will find two preliminary patches:
>
> * The first one implements abstract interfaces. It is fairly
> straightforward and hopefully complete. I would be very glad if
> someone could review it. Maybe this can be committed into trunk soon?
>
> * The second patch deals with procedure declarations. It is not
> finished yet, but can already handle some basic PROCEDURE statements,
> like in the following example code:

Hi Janus,

at a first glance your code looks really clean.  For a patch submission 
you need at least three more things: firstly testcases, otherwise it is 
hard to tell what you're actually trying to support and if the code 
really works.  See <http://gcc.gnu.org/wiki/HowToPrepareATestcase> for 
detailed information on this.  Testcases go into the directory 
gcc/testsuite/gfortran.dg, where you'll find lots of examples.
Secondly, you should provide a ChangeLog together with your patch. 
Examples can be found in the ChangeLog files.
Thirdly, you should say on which platforms you ran the testsuite to 
confirm that there are no regressions.

A few stylistic remarks:
> +  if (m != MATCH_YES)
> +    {
> +      gfc_error ("Syntax error: Garbage in ABSTRACT INTERFACE statement "
> +		 "at %C");
> +      return MATCH_ERROR;
> +    }

I don't find "Garbage" very user-friendly.  The common style seems to be
"Syntax error in ABSTRACT INTERFACE statement at %C".

> +void copy_formal_args (gfc_symbol *dest, gfc_symbol *src)
> +{
> +  gfc_formal_arglist *head = NULL;
> +  gfc_formal_arglist *tail = NULL;
> +  gfc_formal_arglist *formal_arg = NULL;
> +  gfc_formal_arglist *curr_arg = NULL;
> +  gfc_formal_arglist *formal_prev = NULL;
> +  gfc_namespace *parent_ns = NULL;
> +
> +  /* Save current namespace so we can change it for formal args.  */
> +  parent_ns = gfc_current_ns;

Move the assignment to the declaration, i.e. instead of
 gfc_formal_arglist *parent_ns = NULL;
 parent_ns = gfc_current_ns;
write
 gfc_formal_arglist *parent_ns = gfc_current_ns;

Also, there's not much point in initializing variables to zero if 
they're unconditionally overwritten a few lines later.

> +
> +  /* Create a new namespace, which will be the formal ns (namespace
> +     of the formal args).  */
> +  gfc_current_ns = gfc_get_namespace (parent_ns, 0);
> +  gfc_current_ns->proc_name = dest;
> +   
> +  curr_arg = src->formal;
> +  formal_prev = NULL;
> +  while (curr_arg != NULL)
> +    {
> +      /* Allocate a new struct for the formal arg.  */
> +      formal_arg = gfc_get_formal_arglist ();
> +      
> +      /* Create symbol for the arg.  */
> +      gfc_get_symbol (curr_arg->sym->name, gfc_current_ns, &(formal_arg->sym));
> +      /* May need to copy more info for the symbol.  */
> +      formal_arg->sym->attr = curr_arg->sym->attr;
> +      formal_arg->sym->ts = curr_arg->sym->ts;

The first two comments don't add any information, as they mirror what 
the code does.  Comments should answer questions about why you're doing, 
not about what you're doing.  I'm not sure I understand the third one. 
Are you copying the info or is this a TODO?

> +  /* Store the formal namespace information.  */
> +  if (dest->formal != NULL)
> +  /* The current ns should be that for the dest proc.  */
> +  dest->formal_ns = gfc_current_ns;

It looks like this is misindented.

> +  /* get the type spec. for the procedure interface */
> +  old_loc = gfc_current_locus;
> +  m = match_type_spec (&current_ts, 0);
> +  if (m == MATCH_YES || (m == MATCH_NO && gfc_peek_char() == ')'))
> +    goto got_ts;
> +
> +  gfc_current_locus = old_loc;
> +
> +  /* get name of procedure or abstract interface to inherit interface from */
> +  m = gfc_match_symbol (&s, 1);

Comments should be full sentences, beginning with a capital letter, 
ending in punctuation.

Cheers,
- Tobi