Le 06/04/2015 01:04, Evangelos Drikos a écrit : > > Hi, > > The attached patch, type 0, has been discussed a little at: > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=59016 > > Yet, the final version submitted is slightly different from the ones > discussed and tested in the above link. > > Having read the GNU Coding Conventions, I created a patch using svn diff > (“-up”) and I added a small test case. > > Further, I've run the “check_GNU_style.sh” script to ensure that the source > code meets the GNU style requirements. > > Also, I don’t have DejaGNU installed and thus I think that I cannot run all > the tests. In other words, evaluation is up to the GNU team. > > Finally, it might be obvious that the patch is for trunk (not gcc-4.9.2). > Hello,
first, have you signed the copyright assignment form? See the legal prerequisites section at https://gcc.gnu.org/contribute.html but I think you have read that page already. Regarding the patch, I don't understand why the existing symbol restoration code doesn't work here (see gfc_restore_last_undo_checkpoint, restore_old_symbol). I have to investigate more. For now, I have made below a few comments about the patch in its current state. Most of the comments are purely syntactic: the code you insert should have the same formatting as the rest of the code. Mikael PS: It's better if you manage to install DejaGNU, so that you can run the testsuite yourself. We are already short of manpower. > Index: gcc/fortran/decl.c > =================================================================== > --- gcc/fortran/decl.c (revision 221872) > +++ gcc/fortran/decl.c (working copy) > @@ -4427,6 +4432,40 @@ ok: > gfc_free_data_all (gfc_current_ns); > > cleanup: > + //<pr59016> in gfc_match_data_decl; cleanup the garbages No need to reference the PR, unless the code is so subtle that one has to read the PR to understand it. And don't produce code that subtle. ;-) > + gfc_symbol *csym=NULL; > + if ( (m==MATCH_ERROR) //clean only if stmt not matched and No extra parenthesis around the equality, single spaces around an operator, no space inside parenthesis. > + && (ifunlink!=NULL) //the symbol was indeed linked in chain. Indent the operator the same as its left operand > + && (current_ts.u.derived && > + current_ts.u.derived->name)) No extra parenthesis, operator at the beginning of the line, not at the end. > + { indent the brace by two spaces wrt the preceding indentation. > + const char *pname = current_ts.u.derived->name; > + //In case the dt name is in title instead of lower case. > + if ( current_ts.u.derived->name[0] != > + TOLOWER (current_ts.u.derived->name[0])) > + { > + char iname[129]; iname[128]=0; Indent the inner code two spaces more than the preceding indentation (that of the brace). > + for (int i=0; (i < 128);i++) > + { > + iname[i]=current_ts.u.derived->name[i]; > + if (current_ts.u.derived->name[i]==0) > + break; > + }//for Comment useless. > + iname[0] = TOLOWER (iname[0]); > + pname = iname ; > + }//if > + for (int i=0; i<4;i++) { //try iface=0, 1, 2, and 3 This loop doesn't make sense; gfc_find_symbol's third argument shall be either zero or non-zero, and it doesn't make sense to test both variants in a row. > + gfc_find_symbol (pname, NULL, i, &csym) ; > + if ( csym && csym->generic && > + ( csym->generic->sym == current_ts.u.derived)) > + { > + ifunlink->generic->next=csym->generic->next; > //remove from chain > + csym->generic = old_generic; > //restore old value > + break; > + }//if > + }//for > + }//if > + //</pr59016> > gfc_free_array_spec (current_as); > current_as = NULL; > return m;
