This is the mail archive of the fortran@gcc.gnu.org mailing list for the GNU Fortran 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] |
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
Attachment:
code_20051025.diff
Description: code_20051025.diff
Attachment:
changelog_code_20051025.txt
Description: changelog_code_20051025.txt
Index Nav: | [Date Index] [Subject Index] [Author Index] [Thread Index] | |
---|---|---|
Message Nav: | [Date Prev] [Date Next] | [Thread Prev] [Thread Next] |