Re: [PATCH] Avoid some -Wunreachable-code-ctrl

Re: [PATCH] Avoid some -Wunreachable-code-ctrl

[Mikael Morin]

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.

Mikael

Re: [PATCH] Avoid some -Wunreachable-code-ctrl

[Richard Biener]

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  */

?  Btw, a fallthru would be diagnosed by GCC unless we put

            /* Fallthru  */

here.  Maybe renaming WALK_SUBEXPR_TAIL to WALK_SUBEXPR_WITH_CONTINUE
or WALK_SUBEXPR_BY_TAIL_RECURSING or WALK_SUBEXPR_TAILRECURSE would
be more obvious?

Thanks,
Richard.

Re: [PATCH] Avoid some -Wunreachable-code-ctrl

[Mikael Morin]

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.

Thanks.

Re: [PATCH] Avoid some -Wunreachable-code-ctrl

[Richard Biener]

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 <rguenther@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  <rguenther@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