This is the mail archive of the fortran@gcc.gnu.org mailing list for the GNU Fortran project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]