[Patch, Fortran] PR 86935: Bad locus in ASSOCIATE statement

[Janus Weil]

Hi all,

this simple patch improves some of the diagnostics for invalid
ASSOCIATE statements:

https://github.com/janusw/gcc/commit/2f484479c741abddc8ac473cb0c1b9010397e006

In particular it gives a more precise locus and improved wording,
which is achieved basically by splitting a 'gfc_match' into two
separate ones. Regtests cleanly on x86_64-linux-gnu. Ok for trunk?

Cheers,
Janus

[Janus Weil]

Am Di., 14. Aug. 2018 um 19:44 Uhr schrieb Bernhard Reutner-Fischer

Janus,

Post by Janus Weil
Hi all,
this simple patch improves some of the diagnostics for invalid
https://github.com/janusw/gcc/commit/2f484479c741abddc8ac473cb0c1b9010397e006

Please do not send external references but the patch itself for posterity.

"git diff pr86935~1 pr86935 > pr86935.diff"
See attachment.

I think gcc would profit massively not only from having its primary
repository in git, but also having a proper review system
(github/gitlab give great examples, but there are lots of others by
now). Sending patches to mailing lists is so archaic and cumbersome.

Just my opinion, though ...

Cheers,
Janus

[Fritz Reese]

Post by Janus Weil
[...]
I think gcc would profit massively not only from having its primary
repository in git, but also having a proper review system
(github/gitlab give great examples, but there are lots of others by
now). Sending patches to mailing lists is so archaic and cumbersome.
Just my opinion, though ...

I agree!

[Janus Weil]

ping!

Post by Janus Weil

Post by Janus Weil
this simple patch improves some of the diagnostics for invalid
https://github.com/janusw/gcc/commit/2f484479c741abddc8ac473cb0c1b9010397e006

Please do not send external references but the patch itself for posterity.

"git diff pr86935~1 pr86935 > pr86935.diff"
See attachment.

[Bernhard Reutner-Fischer]

ping!
Am Di., 14. Aug. 2018 um 21:40 Uhr schrieb Janus Weil

Post by Janus Weil

Post by Janus Weil
this simple patch improves some of the diagnostics for invalid

https://github.com/janusw/gcc/commit/2f484479c741abddc8ac473cb0c1b9010397e006

Please do not send external references but the patch itself for

posterity.

Post by Janus Weil
"git diff pr86935~1 pr86935 > pr86935.diff"
See attachment.

+++ b/gcc/fortran/match.c
@@ -1889,15 +1889,19 @@ gfc_match_associate (void)
gfc_association_list* a;

/* Match the next association. */
- if (gfc_match (" %n => %e", newAssoc->name, &newAssoc->target)
- != MATCH_YES)
+ if (gfc_match (" %n => ", newAssoc->name) != MATCH_YES)

Since you match " %e", I.e. with leading optional space, I'd omit the trailing blank in " %n => ".

With that change the patch looks ok to me, but I cannot approve it.

Cheers,


+ {
+ gfc_error ("Expected association at %C");
+ goto assocListError;
+ }
+
+ if (gfc_match (" %e", &newAssoc->target) != MATCH_YES)
{

[Thomas Koenig]

Hi Janus,

Post by Janus Weil

Janus,

Post by Janus Weil
Hi all,
this simple patch improves some of the diagnostics for invalid
https://github.com/janusw/gcc/commit/2f484479c741abddc8ac473cb0c1b9010397e006

Please do not send external references but the patch itself for posterity.

"git diff pr86935~1 pr86935 > pr86935.diff"
See attachment.

The patch is OK; you might want to take Bernhard's remark about
the trailing space after "%e" into account.

Regards

Thomas

[Janus Weil]

Am Mi., 22. Aug. 2018 um 17:46 Uhr schrieb Thomas Koenig

Post by Thomas Koenig
Hi Janus,

Post by Janus Weil

Janus,

Post by Janus Weil
Hi all,
this simple patch improves some of the diagnostics for invalid
https://github.com/janusw/gcc/commit/2f484479c741abddc8ac473cb0c1b9010397e006

Please do not send external references but the patch itself for posterity.

"git diff pr86935~1 pr86935 > pr86935.diff"
See attachment.

The patch is OK; you might want to take Bernhard's remark about
the trailing space after "%e" into account.

I have incorporated Bernhard's remark and committed as r263787.

Thanks everyone!

Cheers,
Janus

[Bernhard Reutner-Fischer]

Post by Janus Weil
Am Mi., 22. Aug. 2018 um 17:46 Uhr schrieb Thomas Koenig

Post by Thomas Koenig
Hi Janus,

Post by Janus Weil

Janus,

Post by Janus Weil
Hi all,
this simple patch improves some of the diagnostics for invalid
https://github.com/janusw/gcc/commit/2f484479c741abddc8ac473cb0c1b9010397e006

Please do not send external references but the patch itself for posterity.

"git diff pr86935~1 pr86935 > pr86935.diff"
See attachment.

The patch is OK; you might want to take Bernhard's remark about
the trailing space after "%e" into account.

I have incorporated Bernhard's remark and committed as r263787.

While rebasing my fortran-fe-stringpool branch i spotted one
(pre-existing) possible inconsistency that i did overlook back then:

gfc_match_associate () reads
...
if (gfc_match (" %e", &newAssoc->target) != MATCH_YES)
{
/* Have another go, allowing for procedure pointer selectors. */
gfc_matching_procptr_assignment = 1;
if (gfc_match (" %e", &newAssoc->target) != MATCH_YES)
{
gfc_error ("Invalid association target at %C");
goto assocListError;
}
gfc_matching_procptr_assignment = 0;
}

i.e. we retry a match, but in the second attempt we turn on procptr
assignment matching and if that works, we turn procptr assignment
matching off again.
But if we fail that retry, we forget to turn it off again.
I suppose we should:

$ svn diff -x -p gcc/fortran/match.c
Index: gcc/fortran/match.c
===================================================================
--- gcc/fortran/match.c (revision 264040)
+++ gcc/fortran/match.c (working copy)
@@ -1898,13 +1898,16 @@ gfc_match_associate (void)
if (gfc_match (" %e", &newAssoc->target) != MATCH_YES)
{
/* Have another go, allowing for procedure pointer selectors. */
+ match m;
+
gfc_matching_procptr_assignment = 1;
- if (gfc_match (" %e", &newAssoc->target) != MATCH_YES)
+ m = gfc_match (" %e", &newAssoc->target);
+ gfc_matching_procptr_assignment = 0;
+ if (m != MATCH_YES)
{
gfc_error ("Invalid association target at %C");
goto assocListError;
}
- gfc_matching_procptr_assignment = 0;
}
newAssoc->where = gfc_current_locus;


Untested. Maybe someone wants to give it a whirl...
If it wrecks havoc then leaving it set deliberately deserves at least a comment.

PS: It would be nice to get rid of gfc_matching_procptr_assignment,
gfc_matching_ptr_assignment, gfc_matching_prefix, FWIW.
cheers,

Post by Janus Weil
Thanks everyone!
Cheers,
Janus