On Tue, 30 Nov 2021, Mikael Morin wrote:
> On 30/11/2021 14:25, Richard Biener wrote:
> > On Tue, 30 Nov 2021, Mikael Morin wrote:
> >
> >> Le 29/11/2021 ? 16:03, Richard Biener via Gcc-patches a ?crit :
> >>> diff --git a/gcc/fortran/frontend-passes.c b/gcc/fortran/frontend-passes.c
> >>> index f5ba7cecd54..16ee2afc9c0 100644
> >>> --- a/gcc/fortran/frontend-passes.c
> >>> +++ b/gcc/fortran/frontend-passes.c
> >>> @@ -5229,7 +5229,6 @@ gfc_expr_walker (gfc_expr **e, walk_expr_fn_t
> >>> exprfn,
> >>> void *data)
> >>> case EXPR_OP:
> >>> WALK_SUBEXPR ((*e)->value.op.op1);
> >>> WALK_SUBEXPR_TAIL ((*e)->value.op.op2);
> >>> - break;
> >>> case EXPR_FUNCTION:
> >>> for (a = (*e)->value.function.actual; a; a = a->next)
> >>> WALK_SUBEXPR (a->expr);
> >>
> >> I?m uncomfortable with the above change.
> >> It makes it look like there is a fall through, but there is not.
> >> Maybe inline the macro to make the continue explicit, or use WALK_SUBEXPR
> >> instead of WALK_SUBEXPR_TAIL and hope the compiler will do the tail call
> >> optimization.
> >
> > Ah, it follows the style in tree.c:walk_tree_1 where break was used
> > inconsistently after WALK_SUBTREE_TAIL which was then more obvious
> > to me to clean up. I didn't realize the fortran FE only had a
> > single WALK_SUBEXPR_TAIL.
> >
> > I'm not sure inlining will make the situation more clear, for
> > sure using WALK_SUBEXPR would but it might loose the tailcall.
> >
> > Would you accept an additional comment after WALK_SUBEXPR_TAIL like
> >
> > case EXPR_OP:
> > WALK_SUBEXPR ((*e)->value.op.op1);
> > WALK_SUBEXPR_TAIL ((*e)->value.op.op2);
> > /* tail-recurse */
> >
> My preference would be a gcc_unreachable() or something similar, but I
> understand it would get a warning as well?
>
> Without better idea, I?m fine with an even more explicit comment:
>
> /* No fallthru because of the tail recursion above. */
>
> > ? Btw, a fallthru would be diagnosed by GCC unless we put
> >
> > /* Fallthru */
> >
> > here.
> Sure, but my main concern was misreading from programmers (including me),
> which is not diagnosed by compilers.
>
> > Maybe renaming WALK_SUBEXPR_TAIL to WALK_SUBEXPR_WITH_CONTINUE
> > or WALK_SUBEXPR_BY_TAIL_RECURSING or WALK_SUBEXPR_TAILRECURSE would
> > be more obvious?
> >
> I think the comment above would be enough.
Installed as follows.
Richard.
>From e5c2a436ef7596d254ffefd279742382b1ff546b Mon Sep 17 00:00:00 2001
From: Richard Biener <rguent...@suse.de>
Date: Tue, 30 Nov 2021 15:25:17 +0100
Subject: [PATCH] Add comment to indicate tail recursion
To: gcc-patches@gcc.gnu.org
My previous change removed an unreachable break; there (an
unreachable continue; would have been more to the point). The
following re-adds a comment explaining that WALK_SUBEXPR_TAIL
does not fall through but tail recurses.
2021-11-30 Richard Biener <rguent...@suse.de>
gcc/fortran/
* frontend-passes.c (gfc_expr_walker): Add comment to
indicate tail recursion.
---
gcc/fortran/frontend-passes.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/gcc/fortran/frontend-passes.c b/gcc/fortran/frontend-passes.c
index 16ee2afc9c0..4764c834f4f 100644
--- a/gcc/fortran/frontend-passes.c
+++ b/gcc/fortran/frontend-passes.c
@@ -5229,6 +5229,7 @@ gfc_expr_walker (gfc_expr **e, walk_expr_fn_t exprfn,
void *data)
case EXPR_OP:
WALK_SUBEXPR ((*e)->value.op.op1);
WALK_SUBEXPR_TAIL ((*e)->value.op.op2);
+ /* No fallthru because of the tail recursion above. */
case EXPR_FUNCTION:
for (a = (*e)->value.function.actual; a; a = a->next)
WALK_SUBEXPR (a->expr);
--
2.31.1
