[Patch, fortran] [0/5] PR54730 ICE: confused by type-like fonctions

Tobias Burnus burnus@net-b.de
Fri Feb 22 15:23:00 GMT 2013


Mikael Morin wrote:
> Hello, this is a fix for cases like:
>
> program main
>    implicit none
>    intrinsic :: real
>    print *,(/ real(a = 1) /)
> end
>
> where `real(a = 1)' is initially parsed as a typespec, creating
> a symbol for 'a' along the way.  The match fails, and then it is parsed
> as a constructor element and accepted that way.  However, accepting the
> statement implies accepting all the symbols created so far including 'a',
> which is wrong and leads to the ICE.
> [...]
> This makes backporting a bit more difficult unfortunately; I will submit the (yet nonexisting) backport
> patches separately.

I know that this PR is a 4.6/4.7/4.8 regression and that it presumably 
comes from a real-world code; still, given that one can relatively 
simple work around the issue and that the patch is not tiny (though not 
very complicated either), I wonder whether one should only fix it on the 
4.8 trunk.

> Bootstrap-asan'ed and regression tested on x86_64-unknown-linux-gnu.
> OK for trunk?

It looks mostly okay.

However, I somehow do not like some of names of the new 
procedures/global vars. I find the new "single_undo_checkpoint_p" clear, 
but, without the context of this patch, I presumably had no idea what a 
"checkpoint" means when reading gfortran.h:

+void gfc_new_checkpoint (gfc_change_set &);
+void gfc_drop_last_checkpoint (void);
+void gfc_restore_last_checkpoint (void);

Similarly:

+static gfc_change_set change_set_var = { vNULL, vNULL, NULL };
+static gfc_change_set *changes = &change_set_var;

"changes" is a bit too vague for me (though it is not bad) – and 
"change_set_var" doesn't make it clear enough that it is simply a 
variable, which matches the empty default status.

BTW: Can you also change "static .... = {vNULL ...};" into "static const 
.... = {vNULL ...};" to make sure the value is not accidentally changed?


Regarding the naming, can you use a bit more speaking names? For 
instance – without claiming that the naming choice is best: 
"undo_changes" instead of "changes", "emtpy_undo_change_set_var" instead 
of "change_set_var", "gfc_new_undo_checkpoint" instead of 
"gfc_new_checkpoint". It can be also something different, but it should 
imply what they a good for.


To sum up: The patch is okay with the "const" added. I'd prefer some 
"speaking names", but if you cannot come up with one, the patch is also 
acceptable as is.

Tobias



More information about the Gcc-patches mailing list