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

[Tobias Burnus]

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