Re: [Patch,gfortran] ENUMERATIONS and ENUMERATORS

["Gaurav Gautam, Noida" <gauravga at noida dot hcltech dot com>]

Hi Tobi,

I have updated the patch according to your comments.
Also have reg-tested it with the updated test-suite.
I will be sending test-suite tomorrow.

Thanks

Gaurav Gautam
HCL technologies
Noida, India

> 
> > Index: gcc/fortran/arith.c
> > ===================================================================
> > RCS file: /cvs/gcc/gcc/gcc/fortran/arith.c,v
> > retrieving revision 1.31
> > diff -c -3 -p -r1.31 arith.c
> > *** gcc/fortran/arith.c	17 Sep 2005 18:57:58 -0000	1.31
> > --- gcc/fortran/arith.c	16 Oct 2005 07:58:53 -0000
> > *************** gfc_hollerith2logical (gfc_expr * src, i
> > *** 2372,2374 ****
> > --- 2372,2426 ----
> >
> >     return result;
> >   }
> > +
> > +
> > + /* Returns an initializer with the incremented value from the
> > +    last initializer passed as argument. The kind is set as
> gfc_c_int_kind
> > +    kind. If -fshort-enums is given appropriate kind will be selected
> > +    later when all the enumerators have been parsed.
> > +    A warning is issued here itself, if an initializer exceeds
> gfc_c_int_kind.
> > +    Returns an initializer node.  */
> 
> Note: I'm not a native speaker myself.
> 
> "Returns an initializer whose value is one higher than the value of the
> LAST_INITIALIZER argument.  If that is argument is NULL, the initializers
> value will be set to zero.  The initializer's kind will be set to
> gfc_c_int_kind.  If -fshort-enums is given, the appropriate kind will be
> selected later when all enumerators have been parsed.  A warning is issued
> here if an initializer exceeds gfc_c_int_kind."
> 
> I think previous would be more appropriate than last_initializer, but
> maybe a
> native speaker can tell us if they agree?

------> comment incorporated
> 
> > +   if (last_initializer != NULL)
> > +     {
> > +       mpz_add_ui (result->value.integer, last_initializer-
> >value.integer, 1);
> > +       result->where = last_initializer->where;
> > +
> > +       if (gfc_enum_kind_check (result->value.integer,
>               ^^^^^^^^^^^^^^^^^^^
> > +              gfc_c_int_kind) != ARITH_OK)
> 
> Use before declaration.

-----> gfc_enum_kind_check has been removed and gfc_check_integer_range has  been used in its place. Hence the change related to its usage have also been done in decl.c.
> 
> > + /* Wrapper to access the static function gfc_check_integer_range.  */
> > +
> > + arith
> > + gfc_enum_kind_check (mpz_t p, int kind)
> > + {
> > +   return gfc_check_integer_range(p, kind);
> > + }
> 
> Please use gfc_check_integer_range, remove static from its declaration,
> and
> add it to arith.h instead of adding prototypes all over.

-----> comment incorporated.
> 
> > + /* Initializer of the previous enumerator.  */
> > +
> > + static gfc_expr *last_initializer;
> > +
> > + /* Prototype for gfc_free_enum_history. Declaring this in any header
> > +    did not seem appropriate since its mostly used in this file and in
> > +    parse.c.  */
> 
> Put it in parse.h then.  Prototypes should always be in header files,
> otherwise we end up with several places to update them.

-------> comment incorporated. 
> 
> > +   /* Check, if the symbol is not an enumerator.
> > +      This should have ideally been done inside function "gfc_add_type"
> > +      but since the info regarding enum is not stored in "bt", but
> > +      in "symbol_attribute". This has to be done here only, as
> > +      symbol_attribute is not available inside "gfc_add_type".  */
> > +   if (current_ts.type != BT_UNKNOWN
> > +       && (sym->attr.implicit_type == 0
> > +           || !gfc_compare_types (&sym->ts, &current_ts))
> > +       && sym->ts.type != BT_UNKNOWN && sym->attr.enumerator == 1)
> > +     {
> > +       gfc_error ("Symbol '%s' at %L already has basic type of
> ENUMERATOR", sym->name,
> > +                  var_locus);
> > +       return FAILURE;
> > +     }
> 
> Is there a reason to do this here, besides the wording of the error
> message
> (which I don't agree is a good thing, given that ENUMERATOR is not a basic
> type, and ENUMERATORs are INTEGER, PARAMETERs per the standard).
> 

----> the check has been dropped altogather. The existing error handling is capable to handle this.

> > + /* Function to create and update the enumumerator history
> > +    using the information passed as arguments.
> > +    Pointer "max_enum" is also updated, to point to
> > +    enum history node containing largest initializer.
> > +
> > +    Argument1 is the symbol node of enumerator
> > +    Argument2 is the initializer of enumerator.
> 
> "SYM points to the symbol node of the enumerator,
> INIT points to its initializer"

-----> comment incorporated.
> 
> > + static try
> > + create_enum_history(gfc_symbol *sym, gfc_expr *init)
> > + {
> > +   enumerator_history *new = NULL;
> > +   enumerator_history *current_enum_history = NULL;
> > +
> > +   if (sym == NULL || init == NULL)
> > +     {
> > +       gfc_error ("Invalid enum kind history contents");
> > +       return FAILURE;
> > +     }
> > +   new = gfc_getmem (sizeof (enumerator_history));
> > +   if (new == NULL)
> > +     {
> > +       gfc_error ("Cannot allocate memory for enum kind history");
> > +       return FAILURE;
> > +     }
> 
> These should be gcc_assert()s, all these situations shouldn't occur.
> Since
> create_enum_history will then either return SUCCESS, or not return at all
> change its type to void, and do the corresponding changes in the callers.

-------> comment incorporated.
> 
> > +       current_enum_history = enum_history;
> > +       while (current_enum_history->next != NULL)
> > +         current_enum_history = current_enum_history->next;
> > +
> > +       current_enum_history->next = new;
> 
> Please put the blank like after the assignment.  Is there a reason you're
> appending to the end of the list?  Doing this adds a quadratic algorithm
> (I
> can imagine situations where there are lots of ENUMERATORS).

-----> comment incorporated. I wonder how did I missed this myself?

> 
> > + /* Function to free enum kind history.  */
> > +
> > + void
> > + gfc_free_enum_history(void)
> > + {
> > +   enumerator_history *current = NULL;
> > +   current = enum_history;
> > +
> > +   while (current != NULL)
> > +     {
> > +       enum_history = current->next;
> > +       gfc_free (current);
> > +       current = enum_history;
> 
> While I'm pointing out efficiency problems, it would be faster to use a
> local
> variable here, as the compiler doesn't know that gfc_free doesn't modify
> enum_history.  I also think it would be stylistically cleaner.

------> comment incorporated. 
> 
> > +   /* Check if we are parsing an enumeration and the current enumerator
> variable
> > +      has an initializer or not. If it does not has an initializer, the
> initialisation
> > +      value of previous enumerator (stored in "last_initializer") is
> incremented by 1
> > +      and is used to initialise the current enumerator.  */
>                                 ^
> We use the American spelling: "initialize".

-----> comment incorporated.
> 
> > +   if (gfc_current_state () == COMP_ENUM)
> > +     {
> > +       if (initializer == NULL)
> > +         if ((initializer = gfc_enum_initializer (last_initializer,
> old_locus)) == NULL)
> 
> Please no assignments inside boolean expressions.  The additional pair of
> braces doesn't hurt.
> 
> > +       if (initializer->ts.type != BT_INTEGER)
> > +         {
> > +           gfc_error("ENUMERATOR %L should be initialized with integer
> expression",
> > +                    &var_locus);
> 
> "must be", or use the negative form "ENUMERATOR at %L not initialized with
> integer expression".
> 

-----> Done
> > +       /* Store this current initializer, for next enumerator variable
> to be parsed.  */
>                    ^^^^
> "the"
-----> Done

> 
> > *************** match_type_spec (gfc_typespec * ts, int
> > *** 1379,1386 ****
> > --- 1544,1571 ----
> >
> >     gfc_clear_ts (ts);
> >
> > +   if (gfc_match (" enumerator") == MATCH_YES)
> > +     {
> > +       if (gfc_current_state () != COMP_ENUM)
> > +         {
> > +           gfc_error ("ENUM definition statement expected before %C");
> > +           gfc_free_enum_history ();
> > +           return MATCH_ERROR;
> > +         }
> > +       ts->type = BT_INTEGER;
> > +       ts->kind = gfc_c_int_kind;
> > +
> > +       return MATCH_YES;
> > +     }
> > +
> >     if (gfc_match (" integer") == MATCH_YES)
> >       {
> > +       if (gfc_current_state () == COMP_ENUM)
> > +         {
> > +           gfc_error ("syntax error in ENUMERATOR definition at %C");
> > +           gfc_free_enum_history ();
> > +           return MATCH_ERROR;
> > +         }
> >         ts->type = BT_INTEGER;
> >         ts->kind = gfc_default_integer_kind;
> >         goto get_kind;
> 
> This won't work, you're accepting e.g. a REAL declaration.  I really think
> it
> would be simpler if you wrote a matcher for the interior of an ENUM block,
> as
> there are only two allowed statements: ENUMERATOR and END ENUM.  You could
> then do away with an umber of special cases in what follows.
> 

-----> I have written a new matcher function as discussed in our earlier discussion.

> > + /* Resets the kind of each enumerator, kind is selected such that it
> is
> > +    interoperable with corresponding C enumeration type.
> > +
> > +   If -fshort-enums is given, kind is selected such that it is
> > +   interoperable with C enumeration type (with same option on gcc).  */
> > +
> > + static void
> > + reset_enum_kind(void)
> > + {
> > +   enumerator_history *current_history = NULL;
> > +   int kind;
> > +   int i = 0;
> > +
> > +   if (max_enum == NULL || enum_history == NULL)
> > +     return;
> > +
> > +   if (!gfc_option.fshort_enums)
> > +     return;
> > +   else
> > +     {
> > +       i = 0;
> > +       kind = gfc_integer_kinds[i].kind;
> > +     }
> 
> Don't need else.

----------> comment incorporated
> 
> > +
> > +   while (kind < gfc_c_int_kind && (gfc_enum_kind_check (max_enum-
> >initializer->value.integer,
> > +          kind)) != ARITH_OK)
> 
> Break lines differently instead of adding parentheses.

----------> comment incorporated
> 
> > *************** decode_statement (void)
> > *** 205,210 ****
> > --- 209,215 ----
> >         match ("else", gfc_match_else, ST_ELSE);
> >         match ("else where", gfc_match_elsewhere, ST_ELSEWHERE);
> >         match ("else if", gfc_match_elseif, ST_ELSEIF);
> > +       match ("enum , bind (c) %t", gfc_match_enum, ST_ENUM);
> 
> You need a blank on both sides of the letter 'c', and you don't need one
> in
> front of %t.  Out of consisteny reasons, I think you should call
> gfc_match_eos
> in gfc_match_enum, istead of using %t.

----------> comment incorporated
> 
> Sorry for taking so long again.
> 
> Thanks,
> - Tobi

Attachment: code_20051025.diff
Description: code_20051025.diff

Attachment: changelog_code_20051025.txt
Description: changelog_code_20051025.txt