This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [Patch,gfortran] ENUMERATIONS and ENUMERATORS


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 (&current_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, &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).

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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]