[PATCH, Fortran] PROCEDURE declarations

[Janus Weil]

Hi Tobi,
thanks for your very thorough Saturday-night review :)
I've already finished a part of your suggestions, but some need discussion.


> 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.

Well, ok, one *could* do this. OTOH the code seems perfectly clear to
me as it is, and I'm not quite convinced to change this. In
particular, if we implement the remaining bits of the PROCEDURE syntax
(R445 and R451), then we have two more cases. And these cannot be
distinguished by opening parentheses, but only by context, i.e. with
"gfc_current_state" as I do now. But if you insist (and give me some
good reasons), I'll do it.


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

The words "not yet implemented" to my ears do sound like a compiler
deficiency. And to be honest I don't feel like I have to apologize to
the user for not having implemented some feature. If I would, then
each ifort error message should start like "I'm sorry I suck so hard
at F2003" ;)
Seriously though, I'm now using your recent suggestion "Fortran 2003:
procedure pointers at %L are not yet implemented in gfortran.".

And ok, surely I would *like* to have procedure pointers implemented.
But for now I am happy that basic PROCEDURE statements are working.
Better than nothing, don't you think?


> > +{
> > +  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.

I don't see how ENTRYs could break the patch. Do you have an example?


> > +  /* 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().

There already is a "gfc_add_procedure" which sets the sym->attr.proc
field, i.e. does something different than needed here. So should I
create a new function called "gfc_add_proc"?


> > +      /* 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".

I think Tobi B's suggestion to use "not yet implemented" sounds like
the intrinsic procedure as such is not implemented. Any other ideas?
Maybe we just leave out the "yet"? This avoids confusion with "later
in the program ..", and says that it's not supported (i.e. by the
compiler), even though it's defined in the standard.


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

Right, it's just counting the number of identifiers. But in a more
compact way than

> 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);

Well, this whole thing was not my idea. I'm just using the
implementation of set_binding_label.

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

But at resolution stage we have no information of the number of
identifiers, do we?


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

As you mentioned below, the same code occurs in resolve_symbol. I was
not sure if it would be needed at this point, therefore the
redundancy. Apparently it's not needed and I'll remove it.


> 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 guess this is due to your mailreader. I think my indentation is fine.

The new patch is attached, regtested as usual.
Cheers,
Janus



2007-09-02  Janus Weil  <jaydub66@gmail.com>
	    Paul Thomas  <pault@gcc.gnu.org>

	* decl.c (match_procedure_decl,match_procedure_in_interface,
	gfc_match_procedure): Handle PROCEDURE statements.
	* gfortran.h (struct gfc_symbol): New member "gfc_symbol *interface".
	(enum gfc_statement): New element "ST_PROCEDURE".
	(strcut symbol_attribute): New member "unsigned procedure".
	* interface.c (check_interface0): Extended error checking.
	* match.h: Add gfc_match_procedure prototype.
	* parse.c (decode_statement,next_statement,gfc_ascii_statement,
	parse_derived,parse_interface): Implement PROCEDURE statements.
	* resolve.c (resolve_symbol): Ditto.
	* symbol.c (check_conflict): Ditto.
	(copy_formal_args): New function for copying formal argument lists.


2007-09-02  Janus Weil  <jaydub66@gmail.com>
	    Tobias Burnus  <burnus@net-b.de>

	* gfortran.dg/proc_decl_1.f90: New.
	* gfortran.dg/proc_decl_2.f90: New.
	* gfortran.dg/proc_decl_3.f90: New.
	* gfortran.dg/proc_decl_4.f90: New.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: procdecl128023.diff
Type: text/x-patch
Size: 22388 bytes
Desc: not available
URL: <http://gcc.gnu.org/pipermail/gcc-patches/attachments/20070902/3d94c36f/attachment.bin>