Re: [Patch/gfortran] PR18283

[Tobias Schlüter <tobias dot schlueter at physik dot uni-muenchen dot de>]

Paul Thomas wrote:
> Before I do this patch, I just want to pass a couple of remarks on your 
> comments:
> 
> +> + {
>  > + /* Dereference non-charcter scalar dummy arguments. */
>  > + if ((sym->attr.dummy) && (!sym->attr.dimension))
>  > + se->expr = gfc_build_indirect_ref (se->expr);
>  > +
>  > + /* Dereference scalar hidden result. */
>  > + if ((gfc_option.flag_f2c && sym->ts.type == BT_COMPLEX)
>  > + && (sym->attr.function || sym->attr.result)
>  > + && (!sym->attr.dimension))
>  > + se->expr = gfc_build_indirect_ref (se->expr);
>  > +
>  > + /* Dereference non-character pointer variables.
>  > + These must be dummys or results or scalars. */
>  > + if ((sym->attr.pointer || sym->attr.allocatable)
>  > + && ((sym->attr.dummy)
>  > + || (sym->attr.function || sym->attr.result)
>  > + || (!sym->attr.dimension)))
>  > + se->expr = gfc_build_indirect_ref (se->expr);
>  > + }
>  > +
>  > ref = expr->ref;
>  > }
>  >
> 
> I think you're using parentheses much too liberally, making the code 
> harder to
> read.
> 
> I partially disagree. Not only do I find the code easier to understand 
> with the parenthesis but it is completely unambiguous. In fact, the 
> compiler suggested many of them for exactly this reason.

I'm guessing that the parentheses the compiler suggested were "suggest
parentheses around && within ||", and I completely agree with that.

>  We obviously 
> see code in a completely different way.  I remember being struck by 
> this, when you commented on the first version of this section of code. 
> However, I can agree that the singleton variables can be left 
> unbracketed - it doesn't make it any easier to read though.....

Which is more readable and easier modified:
if ((sym->attr.pointer || sym->attr.allocatable)
    && ((sym->attr.dummy)
        || (sym->attr.function || sym->attr.result)
        || (!sym->attr.dimension)))
or
if ((sym->attr.pointer || sym->attr.allocatable)
    && (sym->attr.dummy
        || sym->attr.function || sym->attr.result
        || !sym->attr.dimension))
?
For me it's clearly the second.  (I'll grant you that the parentheses around
the third line add some information.)  Also every pair of parentheses added
means one more pair to account for when checking that the bracketing is indeed
correct or when trying to understand the logic.  Finally consistency with the
surrounding code makes less bracketing preferable.

- Tobi