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