Le 22/02/2013 16:23, Tobias Burnus a écrit :
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.
Yes we have had two major versions with the bug after all.
Let's go for 4.8 only, that's less work for me. :-)
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);
I have tried to find a good balance between descriptiveness and
verboseness. Before settling on those names, I tried (reading my local
dead branches):
gfc_register_undo_level/gfc_unregister_undo_level/?
gfc_push_undo_level/gfc_pop_undo_level/gfc_undo_one_level
Do you prefer any of them? Otherwise I will just replace "checkpoint"
by "undo_checkpoint" as you suggested.
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.
It's the default status, and it is empty at the beginning. But it's not
constant; changed symbols are added to it by default.
Regarding the name "changes", it is made necessary because the symbol
changes and the tentative_tbp_list are packed together, thus the
variable can't be called "changed_syms" any more. If you don't mind
seeing "changed_syms->syms" in the code we can keep the original name.
Otherwise I'm not very inspired. Would you feel more comfortable with
"latest_undo_changes"?
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",
As said above, it's not always empty, so I will make it
"default_undo_change_set_var" (and keep it non-const). For the rest, I
will add "undo_" before "change_set" and before "checkpoint". Sounds good?
Mikael
