[Patch,gfortran] ENUMERATIONS and ENUMERATORS
Gaurav Gautam, Noida
gauravga@noida.hcltech.com
Tue Oct 25 13:13:00 GMT 2005
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, ¤t_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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: code_20051025.diff
Type: application/octet-stream
Size: 21026 bytes
Desc: code_20051025.diff
URL: <http://gcc.gnu.org/pipermail/fortran/attachments/20051025/53d4f8e3/attachment.obj>
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: changelog_code_20051025.txt
URL: <http://gcc.gnu.org/pipermail/fortran/attachments/20051025/53d4f8e3/attachment.txt>
More information about the Fortran
mailing list