Re: pr59016

[Mikael Morin <mikael.morin@sfr.fr>]

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;