Re: Ping**2! Re: [PATCH, Fortran] Extension: AUTOMATIC/STATIC symbol attributes with -fdec-static

[Fritz Reese]

On Wed, Sep 7, 2016 at 9:31 AM, Andre Vehreschild  wrote:
> Hi Fritz,
>
> please note: I do not have official review privileges. So my vote here
> is rather an advise to you and the official reviewers. Often such a
> inofficial review helps to speed things up, because the official ones
> are pointed to the nics and nacs and don't have to bother with the
> minor things.

Andre,

Thank you very much for your comments. I have only been contributing
to GNU/GCC for a year or two and appreciate the advice. I definitely
strive to keep my patches in line with the relevant standards and
style guides. Attached is a replacement for the original patch which
addresses your concerns.


> - Do I understand this correctly: AUTOMATIC and STATIC have to come last,
>   i.e., right before the :: where declaring, e.g., a variable?
Not quite, you are probably misled by this code in decl.c:

> match
> gfc_match_static (void)
> {
...
> gfc_match (" ::");

(And equivalent for gfc_match_automatic.) These are similar to
gfc_match_save, and like gfc_match_save are only called to match
variable attribute specification _statements_, such as:
> SAVE :: x
> AUTOMATIC :: y

STATIC and AUTOMATIC are matched in any order in an attribute
specification _list_, as with SAVE, through the giant switch() earlier
in decl.c/match_attr_spec(). This applies to the following:
> INTEGER, SAVE, DIMENSION(3) :: x
> INTEGER, AUTOMATIC, DIMENSION(3) :: y


> - Running:
>
>   $ contrib/check_GNU_style.sh dec_static.patch
>
>   Reports some style issues in the C code, that should be fixed before
>   commit. (Style in Fortran testcases does not matter that much.)
I was not aware of this script - thanks!


> Please change formatting in a separate patch or not at all (here!).
> This policy is to distinguish cosmetic changes from relevant ones.
Fixed. These changes are usually accidental - I try not to reformat
code that I'm not otherwise touching.


>> diff --git a/gcc/fortran/gfortran.texi b/gcc/fortran/gfortran.texi
...
>> +With @option{-fdec-static} GNU Fortran supports the explicit specification 
>> of
>> +two addition variable attributes: @code{STATIC} and @code{AUTOMATIC}. These
...
> But is it only for variables? Can't it be used for equivalences or
> other constructs, too?
Yes, good point, perhaps 'entities' is a better term here:

+With @option{-fdec-static} GNU Fortran supports the DEC extended attributes
+@code{STATIC} and @code{AUTOMATIC} to provide explicit specification of entity
+storage. [...]

>> diff --git a/gcc/fortran/invoke.texi b/gcc/fortran/invoke.texi
...
>> +@item -fdec-static
>> +@opindex @code{fdec-static}
>> +Enable STATIC and AUTOMATIC as attributes specifying storage location.
>> +STATIC is equivalent to SAVE, and locals are typically AUTOMATIC by default.
>
> Well, this description to me sounds like: "Those attributes are
> useless, because they can be substituted." This is clearly not what you
> intend. I propose to include into the description that with "this
> switch the dec-extension" is available "to explicitly specify the
> storage of entities". Then the last sentence is still a good hint for
> all fortraneers that don't know the extension.

I guess I subconsciously made them sound "useless" because I hoped
users would think twice about using the extensions and use standard
conforming constructs instead. :-)  But, maybe you are right and this
would be clearer:

+@item -fdec-static
+@opindex @code{fdec-static}
+Enable DEC-style STATIC and AUTOMATIC attributes to explicitly specify
+the storage of variables and other objects.


>> diff --git a/gcc/fortran/lang.opt b/gcc/fortran/lang.opt
...
>> +fdec-static
>> +Fortran Var(flag_dec_static)
>> +Enable STATIC and AUTOMATIC attributes.
>
> How about: Enable the dec-extension of STATIC and AUTOMATIC attributes.
> Just a proposal.
How about this, to match invoke.texi:

+fdec-static
+Fortran Var(flag_dec_static)
+Enable DEC-style STATIC and AUTOMATIC attributes.


> -  Please add some testcases where the new error messages are tested.
Yes, good idea! cf. attached for dec_static_3.f90 and
dec_static_4.f90. These tests gave me a chance to realize I should
emit some better error messages so I made a minor change there:

diff --git a/gcc/fortran/decl.c b/gcc/fortran/decl.c
index db431dd..be8e9f7 100644
--- a/gcc/fortran/decl.c
+++ b/gcc/fortran/decl.c
@@ -7838,6 +7838,8 @@ gfc_match_automatic (void)
   switch (m)
   {
   case MATCH_NO:
+break;
+
   case MATCH_ERROR:
 return MATCH_ERROR;

@@ -7856,7 +7858,7 @@ gfc_match_automatic (void)

   if (!seen_symbol)
 {
-  gfc_error ("Expected var-list in AUTOMATIC statement at %C");
+  gfc_error ("Expected entity-list in AUTOMATIC statement at %C");
   return MATCH_ERROR;
 }

... And similar for gfc_match_static. (Nb. "entity-list" was chosen to
correspond with the "save-entity-list" descriptor used by F90 to
specify the SAVE statement.)

Andre, thanks again for your comments. I 

Re: Ping**2! Re: [PATCH, Fortran] Extension: AUTOMATIC/STATIC symbol attributes with -fdec-static

[Andre Vehreschild]

Hi Fritz,

please note: I do not have official review privileges. So my vote here
is rather an advise to you and the official reviewers. Often such a
inofficial review helps to speed things up, because the official ones
are pointed to the nics and nacs and don't have to bother with the
minor things.

So here it comes:

- Do I understand this correctly: AUTOMATIC and STATIC have to come last,
  i.e., right before the :: where declaring, e.g., a variable?


- Running:

  $ contrib/check_GNU_style.sh dec_static.patch

  Reports some style issues in the C code, that should be fixed before
  commit. (Style in Fortran testcases does not matter that much.)


- I have deleted huge parts of the diff and just kept the parts I had a
  question/remark for:

> diff --git a/gcc/fortran/gfortran.texi b/gcc/fortran/gfortran.texi
> index b34ae86..a0cf78b 100644
> --- a/gcc/fortran/gfortran.texi
> +++ b/gcc/fortran/gfortran.texi
> @@ -2120,7 +2121,6 @@ consider @code{BACKSPACE} or @code{REWIND} to properly 
> position
>  the file before the EOF marker.  As an extension, the run-time error may
>  be disabled using -std=legacy.
>  
> -

Please change formatting in a separate patch or not at all (here!).
This policy is to distinguish cosmetic changes from relevant ones.

>  @node STRUCTURE and RECORD
>  @subsection @code{STRUCTURE} and @code{RECORD}
>  @cindex @code{STRUCTURE}
> @@ -2420,6 +2420,53 @@ here:
>@tab @code{--} @tab @code{FLOATI} @tab @code{FLOATJ} @tab @code{FLOATK}
>  @end multitable
>  
> +@node AUTOMATIC and STATIC attributes
> +@subsection @code{AUTOMATIC} and @code{STATIC} attributes
> +@cindex variable attributes
> +@cindex @code{AUTOMATIC}
> +@cindex @code{STATIC}
> +
> +With @option{-fdec-static} GNU Fortran supports the explicit specification of
> +two addition variable attributes: @code{STATIC} and @code{AUTOMATIC}. These

two additional variable ...
^^ 

But is it only for variables? Can't it be used for equivalences or
other constructs, too?

> diff --git a/gcc/fortran/invoke.texi b/gcc/fortran/invoke.texi
> index 15c131a..a5da59e 100644
> --- a/gcc/fortran/invoke.texi
> +++ b/gcc/fortran/invoke.texi
> @@ -255,6 +255,11 @@ instead where possible.
>  Enable B/I/J/K kind variants of existing integer functions (e.g. BIAND, 
> IIAND,
>  JIAND, etc...). For a complete list of intrinsics see the full documentation.
>  
> +@item -fdec-static
> +@opindex @code{fdec-static}
> +Enable STATIC and AUTOMATIC as attributes specifying storage location.
> +STATIC is equivalent to SAVE, and locals are typically AUTOMATIC by default.

Well, this description to me sounds like: "Those attributes are
useless, because they can be substituted." This is clearly not what you
intend. I propose to include into the description that with "this
switch the dec-extension" is available "to explicitly specify the
storage of entities". Then the last sentence is still a good hint for
all fortraneers that don't know the extension.

> +
>  @item -fdollar-ok
>  @opindex @code{fdollar-ok}
>  @cindex @code{$}
> diff --git a/gcc/fortran/lang.opt b/gcc/fortran/lang.opt
> index 8ec5400..260512d 100644
> --- a/gcc/fortran/lang.opt
> +++ b/gcc/fortran/lang.opt
> @@ -432,6 +432,10 @@ fdec-structure
>  Fortran
>  Enable support for DEC STRUCTURE/RECORD.
>  
> +fdec-static
> +Fortran Var(flag_dec_static)
> +Enable STATIC and AUTOMATIC attributes.

How about: Enable the dec-extension of STATIC and AUTOMATIC attributes.
Just a proposal.

> +
>  fdefault-double-8
>  Fortran Var(flag_default_double)
>  Set the default double precision kind to an 8 byte wide type.


> diff --git a/gcc/testsuite/gfortran.dg/dec_static_1.f90 
> b/gcc/testsuite/gfortran.dg/dec_static_1.f90
> new file mode 100644
> index 000..4dcfc7c
> --- /dev/null
> +++ b/gcc/testsuite/gfortran.dg/dec_static_1.f90

-  Please add some testcases where the new error messages are tested.

So much from my side. Btw, I haven't applied the patch and tested
whether it runs or collides with other proposed patches. That is
usually done by Dominique and I did not want to waste doing it a second
time.

Regards,
Andre
-- 
Andre Vehreschild * Email: vehre ad gmx dot de 

Ping**2! Re: [PATCH, Fortran] Extension: AUTOMATIC/STATIC symbol attributes with -fdec-static

[Fritz Reese]

https://gcc.gnu.org/ml/fortran/2016-08/msg00173.html
On Mon, Aug 29, 2016 at 8:36 AM, Fritz Reese  wrote:
>
> https://gcc.gnu.org/ml/fortran/2016-08/msg00077.html
> On Wed, Aug 17, 2016 at 7:20 AM, Fritz Reese  wrote:
> > This patch extends the GNU Fortran front-end to add support for
> > DEC-style AUTOMATIC and STATIC symbol attributes with a new flag
> > -fdec-static, allowing explicit control of variable storage. AUTOMATIC
> > local variables are placed on the stack, whereas STATIC variables are
> > placed in static storage.
>
> Patch still pending review.
>
> > Bootstraps and regtests on x86_64-redhat-linux. Questions, comments,
> > and critique welcome. Ok for trunk?
> ...
> >
> > 08-17-2016  Fritz Reese  
> >
> > gcc/fortran/
> > * lang.opt, invoke.texi, gfortran.texi: New flag -fdec-static.
> > * options.c (set_dec_flags): Set -fdec-static with -fdec.
> > * gfortran.h (symbol_attribute): New attribute AUTOMATIC.
> > * gfortran.h (gfc_add_automatic): New prototype.
> > * match.h (gfc_match_automatic, gfc_match_static): New functions.
> > * decl.c (gfc_match_automatic, gfc_match_static): Ditto.
> > * symbol.c (gfc_add_automatic): Ditto.
> > * decl.c (match_attr_spec): Match AUTOMATIC and STATIC decls.
> > * parse.c (decode_specification_statement, decode_statement): Ditto.
> > * resolve.c (apply_default_init_local, resolve_fl_variable_derived,
> > resolve_symbol): Support for automatic attribute.
> > * symbol.c (check_conflict, gfc_copy_attr, gfc_is_var_automatic):
> > Ditto.
> > * trans-decl.c (gfc_finish_var_decl): Ditto.
> >
> > gcc/testsuite/gfortran.dg/
> > * dec_static_1.f90, dec_static_2.f90: New testcases.
>


---
Fritz Reese