This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [Patch,gfortran] ENUMERATIONS and ENUMERATORS
- From: Tobias Schlüter <tobias dot schlueter at physik dot uni-muenchen dot de>
- To: "Gaurav Gautam, Noida" <gauravga at noida dot hcltech dot com>
- Cc: fortran at gcc dot gnu dot org, gcc-patches at gcc dot gnu dot org,paul at codesourcery dot com
- Date: Fri, 21 Oct 2005 01:01:12 +0200
- Subject: Re: [Patch,gfortran] ENUMERATIONS and ENUMERATORS
- References: <40CC5CC7CDACC048B1299C57025E330C0159ABCE@HSDLNTD1110010.noida.hcltech.com>
Gaurav Gautam, Noida wrote:
> hi,
>
> I have updated the patch according your comments.
> please find code diff and change log with this mail.
>
>
>>I also have a few remarks WRT the code which would also apply to a revised
>>version:
>>- there are a number of cases where you're not following the coding standards
>>(trailing whitespace, spaces around '->', maybe others)
>
>
> --changes made for this comments
>
>
>>- I think the function prototypes you added can safely be put into parse.h,
>>otherwise don't hesistate to put them in gfortran.h
>
>
> --prototypes added to these two files.
>
>
>>- the logic here would be much easier if you moved the check for COMP_ENUM
>>before the other if
>>
>>> if (d == DECL_NONE || d == DECL_COLON)
>>>! {
>>>! if (gfc_current_state () == COMP_ENUM)
>>>! {
>>>! t = gfc_add_flavor (¤t_attr, FL_PARAMETER, NULL, NULL);
>>>! if (t == FAILURE)
>>>! {
>>>! m = MATCH_ERROR;
>>>! goto cleanup;
>>>! }
>>>! current_attr.enumerator = 1;
>>>! }
>>>! break;
>>>! }
>>>! else if (gfc_current_state () == COMP_ENUM)
>>>! {
>>>! gfc_error ("Enumerator cannot have attributes %C");
>>>! return MATCH_ERROR;
>>>! }
>>
>>the same goes for the change to variable_decl.
>
>
> --changed accordingly
>
>
>
>>I'm wondering if using gfc_match_data_decl makes much sense given the very
>>restricted syntax that is allowed inside enumeration declarations.
>
>
> --since enumerators have implemted as integer constants, so most of the functionality of
> gfc_data_decl has been used. writing a different function didnt seem appropriate to me, since this would have lead to a large amount of repeated code.
>
> --gaurav gautam
>
>
>
> 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?
> + 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.
> + /* 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.
> + /* 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.
> + /* 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).
> + /* 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"
> + 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.
> + 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).
> + /* 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.
> + /* 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".
> + 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".
> + /* Store this current initializer, for next enumerator variable to be parsed. */
^^^^
"the"
> *************** 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.
> + /* 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.
> +
> + 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.
> *************** 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.
Sorry for taking so long again.
Thanks,
- Tobi