Re: [PATCH, Fortran] PROCEDURE declarations

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

Janus Weil wrote:
> So I introduced two new functions "gfc_match_procedure_decl" and
> "gfc_match_procedure_in_interface" to separately match PROCEDURE
> statements in the situations described above, and kept
> "gfc_match_procedure" just for deciding which one to use (from there
> one could later also call stuff like "gfc_match_procedure_component").
> The new patch is attached. I hope you agree with me that this makes
> sense. If you have any concerns (or further comments) please let me
> know.

This is a very good idea, the previous versions of 
gfc_match_procedure_decl scared me a lot as they seemed so big and 
deeply nested :-)

Actually, I think it would make even more sense to introduce two 
different statement types, say ST_PROCEDURE (R1206) and 
ST_PROCEDURE_DECL (R1211).  Then distinguish between the two in 
decode_statement by the opening parentheses required in R1211.  This 
would allow you to put fit the check whether the statement is allowed 
into parse.c's provisions.

> +   /* TODO: Implement procedure pointers.  */
> +  if (attr->procedure && attr->pointer)
> +    {
> +      gfc_error ("Procedure pointers used at %L are "
> +		"not yet implemented", where);
> +      return FAILURE;
> +    }
> +

The wording should make very explicit that this is a compiler 
deficiency, maybe use "sorry"?  I also feel that this is an omission 
that limits the usefulness of this language feature by quite a lot, but 
I only started reading about F2K's features, so I may be misestimating.

> +/* Copy the formal args from an existing symbol, src, into a new
> +   symbol, dest.  New formal args are created, and the description of
> +   each arg is set according to the existing ones.  This function is
> +   used when creating procedure declaration variables from a procedure
> +   declaration statement (see match_proc_decl()) to create the formal
> +   args based on the args of a given named interface.  */
> +
> +void copy_formal_args (gfc_symbol *dest, gfc_symbol *src)

void
gfc_copy_formal_args (...)

interface.c may be a better place for this function.

>  /* Builds the parameter list for the iso_c_binding procedure
>     c_f_pointer or c_f_procpointer.  The old_sym typically refers to a
> Index: gcc/fortran/decl.c
> ===================================================================
> --- gcc/fortran/decl.c	(revision 128012)
> +++ gcc/fortran/decl.c	(working copy)
> @@ -3633,6 +3633,249 @@ gfc_match_suffix (gfc_symbol *sym, gfc_s
>  }
>  
>  
> +/* General matcher for PROCEDURE declarations.  */
> +
> +match
> +gfc_match_procedure (void)
> +{
> +  match m;
> +
> +  switch (gfc_current_state ())
> +    {
> +    case COMP_NONE:
> +    case COMP_PROGRAM:
> +    case COMP_MODULE:
> +    case COMP_SUBROUTINE:
> +    case COMP_FUNCTION:
> +      m = gfc_match_procedure_decl ();
> +      break;
> +    case COMP_INTERFACE:
> +      m = gfc_match_procedure_in_interface ();
> +      break;
> +    case COMP_DERIVED:
> +      gfc_error ("Procedure components used at %C are not yet implemented");
> +      return MATCH_ERROR;
> +    default:
> +      return MATCH_NO;
> +    }
> +
> +  if (gfc_notify_std (GFC_STD_F2003, "Fortran 2003: PROCEDURE statement at %C")
> +      == FAILURE)
> +    return MATCH_ERROR;

If we get here with m == MATCH_NO, it doesn't make sense to give this 
message.

> +/* Match a PROCEDURE declaration.  */
> +
> +match
> +gfc_match_procedure_decl (void)

static match
match_procedure_decl (void)

Likewise for gfc_match_procedure_in_interface.

> +{
> +  match m;
> +  locus old_loc, entry_loc;

The naming of the variable reminds me: you should add testcases that 
everything works with ENTRYs, they have a tendency of breaking 
assumptions everywhere.

> +  /* 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;

Need to deal with m == MATCH_ERROR

> +  gfc_current_locus = old_loc;
> +
> +  /* Get the name of the procedure or abstract interface to inherit interface from.  */

Overlong line.

> +  /* Various interface checks.  */

Move them to a gfc_add_procedure function in symbol.c.
> +  if (proc_if)
> +    {
> +      if (proc_if->generic)
> +	{
> +	  gfc_error ("Interface '%s' at %C may not be generic", proc_if->name);
> +	  return MATCH_ERROR;
> +	}
> +      if (proc_if->attr.proc == PROC_ST_FUNCTION)
> +	{
> +	  gfc_error ("Interface '%s' at %C may not be a statement function",
> +		    proc_if->name);
> +	  return MATCH_ERROR;
> +	}

This particular case should be picked up by the call to check_conflict 
in your new gfc_add_procedure().

> +      /* TODO: Allow intrinsics with gfc_intrinsic_actual_ok (proc_if->name, 0)
> +      after PR33162 is fixed.  */
> +      if (proc_if->attr.intrinsic)
> +	{
> +	  gfc_error ("Intrinsic procedure '%s' not yet supported "
> +		    "in PROCEDURE statement at %C", proc_if->name);
> +	  return MATCH_ERROR;
> +	}
> +    }

Again, make clear that this is a compiler deficiency.  People may wonder 
if "not yet supported" may mean "at a later point in the program, this 
would work".

> +  /* Get procedure symbols.  */
> +  for(num=1;;num++)

I'm wondering if there's a more C-ish way of writing this, num is not at 
all related to the loop logic.

Maybe
num = 0;
do
  {
     num++;
     ...
  }
while (1);

Actually, looking over the code in set_binding_label(), this counting 
stuff is misleading: it is only needed to see if there are multiple 
identifiers associated with the same C name.  I.e. this stuff could be 
reworked to use a boolean flag, modifying set_binding_label() and its 
callers along the way, i.e. this would look like
only_idetifier = truee;
do
  {
    ...
    set_binding_label (..., flag);
    flag = false;
  }
while (1);

Another solution would be to move this particular check to the 
resolution stage.  While even cleaner, this also would mean more work 
for you.

> +      /* Set interface.  */
> +      if (proc_if != NULL)
> +	sym->interface = proc_if;
> +      else if (current_ts.type != BT_UNKNOWN)
> +	{
> +	  sym->interface = gfc_new_symbol ("", gfc_current_ns);
> +	  sym->interface->ts = current_ts;
> +	  sym->interface->attr.function = 1;
> +	  sym->ts = sym->interface->ts;
> +	  sym->attr.function = sym->interface->attr.function;
> +	}
> +
> +      if (sym->interface && sym->interface->attr.if_source)
> +	{
> +	  sym->ts = sym->interface->ts;
> +	  sym->attr.function = sym->interface->attr.function;
> +	  sym->attr.subroutine = sym->interface->attr.subroutine;
> +	  copy_formal_args (sym, sym->interface);
> +	}

I take it there can be no overlap between the last case of the first if, 
and the second if?  Then this should be an else if.  Otherwise, the code 
is confusing, as flags may be set and reset along the way.

> +
> +      if (gfc_match_eos () == MATCH_YES)
> +	break;
> +      if (gfc_match_char (',') != MATCH_YES)
> +	goto syntax;
> +    }
> +
> +  return MATCH_YES;

Move the return below if (gfc_match_eos () == MATCH_YES).  That makes 
clear that any other way of leaving the loop is erroneous.

Perhaps due to our (IMO braindead) use of tabs and it's interactions 
with quoting and the patch format in my mailreader, in the following 
your indentation constantly seems to be off by one.

> +  for(;;)
> +    {
> +      m = gfc_match_name (name);
> +      if (m == MATCH_NO)
> +	goto syntax;
> +      if (m != MATCH_YES)
> +	return MATCH_ERROR;

I.e., if (m == MATCH_ERROR).  Also, make this an else if.

> +      if (gfc_get_symbol (name, gfc_current_ns->parent, &sym))
> +	return MATCH_ERROR;
> +
> +      if (gfc_add_interface (sym) == FAILURE)
> +	return MATCH_ERROR;
> +
> +      sym->attr.procedure = 1;

In this particular case, I can't think of a case where 
gfc_add_procedure() may be doing better than this, but I can't see where 
it could hurt either, and in the interest of future maintainability, I 
think it's better to call it.

> Index: gcc/fortran/resolve.c
> ===================================================================
> --- gcc/fortran/resolve.c	(revision 128012)
> +++ gcc/fortran/resolve.c	(working copy)
> @@ -7362,6 +7362,25 @@ resolve_symbol (gfc_symbol *sym)
>  	}
>      }
>  
> +  if (sym->attr.procedure && sym->interface
> +      && sym->attr.if_source != IFSRC_DECL)
> +    {
> +      /* Get the attributes from the interface (now resolved).  */
> +      if (sym->interface->attr.if_source || sym->interface->attr.intrinsic)
> +	{
> +	  sym->ts = sym->interface->ts;
> +	  sym->attr.function = sym->interface->attr.function;
> +	  sym->attr.subroutine = sym->interface->attr.subroutine;
> +	  copy_formal_args (sym, sym->interface);
> +	}
> +      else if (sym->interface->name[0] != '\0')
> +	{
> +	  gfc_error ("Interface '%s' of procedure '%s' at %L must be explicit",
> +		    sym->interface->name, sym->name, &sym->declared_at);
> +	  return;
> +	}
> +    }
> +

You're duplicating the code from gfc_match_procedure_decl here. 
Couldn't procedure declarations rely on the resolution stage for this as 
well?

> Index: gcc/fortran/match.h
> ===================================================================
> --- gcc/fortran/match.h	(revision 128012)
> +++ gcc/fortran/match.h	(working copy)
> @@ -134,6 +134,9 @@ match gfc_match_old_kind_spec (gfc_types
>  match gfc_match_end (gfc_statement *);
>  match gfc_match_data_decl (void);
>  match gfc_match_formal_arglist (gfc_symbol *, int, int);
> +match gfc_match_procedure (void);
> +match gfc_match_procedure_decl (void);
> +match gfc_match_procedure_in_interface (void);

Remove prototypes for soon-static functions.

Overall, this is very close to ready.  I'd appreciate you posting a 
version with these changes incorporated.  Once Tobias B believes the 
language support is complete, I don't doubt that it will be ok from my POV.

Thanks for bearing with me,
- Tobi