[Bug fortran/62270] New: -Wlogical-not-parentheses warnings

[Bug fortran/62270] New: -Wlogical-not-parentheses warnings

[mpolacek at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=62270

            Bug ID: 62270
           Summary: -Wlogical-not-parentheses warnings
           Product: gcc
           Version: 5.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: fortran
          Assignee: unassigned at gcc dot gnu.org
          Reporter: mpolacek at gcc dot gnu.org

-Wlogical-not-parentheses detected these two issues:

interface.c:compare_parameter
2013   /* F2008, 12.5.2.5; IR F08/0073.  */
2014   if (formal->ts.type == BT_CLASS && formal->attr.class_ok
2015       && actual->expr_type != EXPR_NULL
2016       && ((CLASS_DATA (formal)->attr.class_pointer
2017            && !formal->attr.intent == INTENT_IN)

trans-expr.c:gfc_conv_procedure_call
4445                   if (fsym->attr.optional
4446                       && e->expr_type == EXPR_VARIABLE
4447                       && (!e->ref
4448                           || (e->ref->type == REF_ARRAY
4449                               && !e->ref->u.ar.type != AR_FULL))

But my attempts to fix these failed.
The first one should likely be "&& formal->attr.intent != INTENT_IN)", but that
regressed gfortran.dg/pointer_intent_7.f90 test.  If I change it to "==", many
tests fail.
For the second one, perhaps just the "!" should be dropped, but I'm not sure
about that, testsuite didn't reveal anything.

Currently this PR is the last thing that is blocking the inclusion of
-Wlogical-not-parentheses in -Wall - I'd appreciate if anyone could look into
this PR.

[Bug fortran/62270] -Wlogical-not-parentheses warnings

[burnus at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=62270

--- Comment #3 from Tobias Burnus <burnus at gcc dot gnu.org> ---
(In reply to Marek Polacek from comment #0)
> -Wlogical-not-parentheses detected these two issues:

I really missed such a warning before; Coverity also detected such issues, but
it has way too much output to dig through all.

> interface.c:compare_parameter
> 2013   /* F2008, 12.5.2.5; IR F08/0073.  */
> 2014   if (formal->ts.type == BT_CLASS && formal->attr.class_ok
> 2015       && actual->expr_type != EXPR_NULL
> 2016       && ((CLASS_DATA (formal)->attr.class_pointer
> 2017            && !formal->attr.intent == INTENT_IN)

Here, it really should be: Print an error when passing a nonpolymophic actual
argument to a polymorphic dummy/formal argument - to avoid modification of the
run-time type. But as that's not possible with INTENT_IN, one can permit it in
that case.

Thus, the obvious patch should be correct:

--- a/gcc/fortran/interface.c
+++ b/gcc/fortran/interface.c
@@ -2016,3 +2016,3 @@ compare_parameter (gfc_symbol *formal, gfc_expr *actual,
       && ((CLASS_DATA (formal)->attr.class_pointer
-          && !formal->attr.intent == INTENT_IN)
+          && formal->attr.intent != INTENT_IN)
           || CLASS_DATA (formal)->attr.allocatable))


> trans-expr.c:gfc_conv_procedure_call
> 4445                   if (fsym->attr.optional
> 4446                       && e->expr_type == EXPR_VARIABLE
> 4447                       && (!e->ref
> 4448                           || (e->ref->type == REF_ARRAY
> 4449                               && !e->ref->u.ar.type != AR_FULL))

In the surrounding code block, one inserts a deallocation call to the data
component of a array descriptor ("free (var->data);") - but if the argument can
be absent ("optional" attribute), one has var == NULL. Thus, one needs to add
an extra check before dereferencing. But that is only needed if the users don't
add array/subcomponent references themselves (e.g."var(:)%subcomp").

Thus, the check is only needed if e->ref == NULL - or one has an (automaically
added) reference to the full array. Again, the obvious patch should be correct:

--- a/gcc/fortran/trans-expr.c
+++ b/gcc/fortran/trans-expr.c
@@ -4592 +4592 @@ gfc_conv_procedure_call (gfc_se * se, gfc_symbol * sym,
-                             && !e->ref->u.ar.type != AR_FULL))
+                             && e->ref->u.ar.type != AR_FULL))

Side remark: I wonder whether BT_CLASS needs some extra care.


> The first one should likely be "&& formal->attr.intent != INTENT_IN)", but
> that regressed gfortran.dg/pointer_intent_7.f90 test.

The problem is that one has two different issues - and now one earlier check
triggers. For
  call bar3p (b)
there are two related issues: One passes a pointer-intent(in) variable to a
pointer-intent(inout) dummy-argument variable. And passes a nonpolymorphic
pointer to a polymorphic dummy-argument variable w/o intent(in).

Seemingly, the second issue is checked before in the code - and didn't trigger.
(Which was a bug.) Thus, simply change the dg-error message.

[Depending what the user intended, one or the other message is more helpful -
hence, it doesn't matter which one is shown.]


I won't be able to package and test this code before this evening; feel free to
package, regtest and submit it yourself.


(In reply to Thomas Koenig from comment #2)
> A longer version would be [...] Is this what is intended here?

Well, the code is bogus - for obvious reasons. The problem was more what is
correct and makes code wise sense - re-writing the existing condition doesn't
help.

[Bug fortran/62270] -Wlogical-not-parentheses warnings

[mpolacek at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=62270

Marek Polacek <mpolacek at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |ASSIGNED
           Assignee|unassigned at gcc dot gnu.org      |mpolacek at gcc dot gnu.org
   Target Milestone|---                         |5.0

--- Comment #4 from Marek Polacek <mpolacek at gcc dot gnu.org> ---
Ok, thanks a lot Tobias, I'll take this one then.

[Bug fortran/62270] -Wlogical-not-parentheses warnings

[mpolacek at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=62270

--- Comment #5 from Marek Polacek <mpolacek at gcc dot gnu.org> ---
Author: mpolacek
Date: Tue Sep  2 16:08:05 2014
New Revision: 214827

URL: https://gcc.gnu.org/viewcvs?rev=214827&root=gcc&view=rev
Log:
    PR fortran/62270
    * interface.c (compare_parameter): Fix condition.
    * trans-expr.c (gfc_conv_procedure_call): Likewise.

    * gfortran.dg/pointer_intent_7.f90: Adjust dg-error.

Modified:
    trunk/gcc/fortran/ChangeLog
    trunk/gcc/fortran/interface.c
    trunk/gcc/fortran/trans-expr.c
    trunk/gcc/testsuite/ChangeLog
    trunk/gcc/testsuite/gfortran.dg/pointer_intent_7.f90

[Bug fortran/62270] -Wlogical-not-parentheses warnings

[mpolacek at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=62270

Marek Polacek <mpolacek at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |RESOLVED
         Resolution|---                         |FIXED

--- Comment #6 from Marek Polacek <mpolacek at gcc dot gnu.org> ---
Fixed.

[Bug fortran/62270] -Wlogical-not-parentheses warnings

[mpolacek at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=62270

--- Comment #7 from Marek Polacek <mpolacek at gcc dot gnu.org> ---
Author: mpolacek
Date: Wed Sep  3 16:04:27 2014
New Revision: 214881

URL: https://gcc.gnu.org/viewcvs?rev=214881&root=gcc&view=rev
Log:
    PR fortran/62270
    * interface.c (compare_parameter): Fix condition.
    * trans-expr.c (gfc_conv_procedure_call): Likewise.

    * gfortran.dg/pointer_intent_7.f90: Adjust dg-error.

Modified:
    branches/gcc-4_9-branch/gcc/fortran/ChangeLog
    branches/gcc-4_9-branch/gcc/fortran/interface.c
    branches/gcc-4_9-branch/gcc/fortran/trans-expr.c
    branches/gcc-4_9-branch/gcc/testsuite/ChangeLog
    branches/gcc-4_9-branch/gcc/testsuite/gfortran.dg/pointer_intent_7.f90

[Bug fortran/62270] -Wlogical-not-parentheses warnings

[mpolacek at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=62270

--- Comment #8 from Marek Polacek <mpolacek at gcc dot gnu.org> ---
Author: mpolacek
Date: Wed Sep  3 17:25:45 2014
New Revision: 214887

URL: https://gcc.gnu.org/viewcvs?rev=214887&root=gcc&view=rev
Log:
    PR fortran/62270
    * interface.c (compare_parameter): Fix condition.

    * gfortran.dg/pointer_intent_7.f90: Adjust dg-error.

Modified:
    branches/gcc-4_8-branch/gcc/fortran/ChangeLog
    branches/gcc-4_8-branch/gcc/fortran/interface.c
    branches/gcc-4_8-branch/gcc/testsuite/ChangeLog
    branches/gcc-4_8-branch/gcc/testsuite/gfortran.dg/pointer_intent_7.f90

[Bug fortran/62270] -Wlogical-not-parentheses warnings

[dcb314 at hotmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=62270

David Binderman <dcb314 at hotmail dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |dcb314 at hotmail dot com

--- Comment #9 from David Binderman <dcb314 at hotmail dot com> ---
> Currently this PR is the last thing that is blocking the inclusion of
> -Wlogical-not-parentheses in -Wall - I'd appreciate if anyone could look
> into this PR.

Maybe brave, maybe reckless ;->

Is a brand new warning ready for the prime time of -Wall ?

It might be a tad more conservative to implement the proposed warning
and not include it in -Wall until some time has elapsed and some user 
feedback has been provided.

For instance, what effect does new flag -Wlogical-not-parentheses have
on building major packages like the Linux kernel, or Fedora,
or others ?

[Bug fortran/62270] -Wlogical-not-parentheses warnings

[mpolacek at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=62270

--- Comment #10 from Marek Polacek <mpolacek at gcc dot gnu.org> ---
(In reply to David Binderman from comment #9)
> Is a brand new warning ready for the prime time of -Wall ?

The warning existed for some time already before it's been added to -Wall.

> It might be a tad more conservative to implement the proposed warning
> and not include it in -Wall until some time has elapsed and some user 
> feedback has been provided.

That's what happened.  And I believe few people noticed the new warning.

> For instance, what effect does new flag -Wlogical-not-parentheses have
> on building major packages like the Linux kernel, or Fedora,
> or others ?

Some of them won't build because of -Werror, I suppose, but that happens every
year when we do the mass rebuilds; -Wall isn't guaranteed to be consistent from
release to release.  It should not have almost any false positives, and there's
always the option to use -Wno-logical-not-parentheses or to wrap LHS in parens
to suppress the warning.