parentheses flag warning

classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|

parentheses flag warning

Fangrui Song via cfe-dev
-Wparentheses

warns for line 3 but not for the ternary expression in line 2.

1 static void foo(int a, int b, int x) {
2    x = (x = 10) ? a : b;
3    if (x = 10) { x = a; } else { x = b; }
4 }

Is this a bug?

Regards,

Billy. 


_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: parentheses flag warning

Fangrui Song via cfe-dev
GCC doesn't seem to warn on this either. On the basis of the original warning I could see how this case could merit similar handling - but I doubt we'd want to add that handling to the existing flag at this point (due to the number of people over the decades that have used the warning for the behavior it currently provides, not expecting the new/additional behavior for the ternary operator or other bool-testing assignment situations (eg: "void f(bool); f(x = 5);")

On Mon, May 18, 2020 at 6:53 AM Billy Araujo via cfe-dev <[hidden email]> wrote:
-Wparentheses

warns for line 3 but not for the ternary expression in line 2.

1 static void foo(int a, int b, int x) {
2    x = (x = 10) ? a : b;
3    if (x = 10) { x = a; } else { x = b; }
4 }

Is this a bug?

Regards,

Billy. 

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: parentheses flag warning

Fangrui Song via cfe-dev
Hi David,

So I guess this is expected behaviour. Thanks.

Regards,

Billy.


On Mon, May 18, 2020 at 6:39 PM David Blaikie <[hidden email]> wrote:
GCC doesn't seem to warn on this either. On the basis of the original warning I could see how this case could merit similar handling - but I doubt we'd want to add that handling to the existing flag at this point (due to the number of people over the decades that have used the warning for the behavior it currently provides, not expecting the new/additional behavior for the ternary operator or other bool-testing assignment situations (eg: "void f(bool); f(x = 5);")

On Mon, May 18, 2020 at 6:53 AM Billy Araujo via cfe-dev <[hidden email]> wrote:
-Wparentheses

warns for line 3 but not for the ternary expression in line 2.

1 static void foo(int a, int b, int x) {
2    x = (x = 10) ? a : b;
3    if (x = 10) { x = a; } else { x = b; }
4 }

Is this a bug?

Regards,

Billy. 

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: parentheses flag warning

Fangrui Song via cfe-dev
On Mon, May 18, 2020 at 8:44 PM Billy Araujo via cfe-dev
<[hidden email]> wrote:

>
> Hi David,
>
> So I guess this is expected behaviour. Thanks.
>
> Regards,
>
> Billy.
>
>
> On Mon, May 18, 2020 at 6:39 PM David Blaikie <[hidden email]> wrote:
>>
>> GCC doesn't seem to warn on this either. On the basis of the original warning I could see how this case could merit similar handling - but I doubt we'd want to add that handling to the existing flag at this point (due to the number of people over the decades that have used the warning for the behavior it currently provides, not expecting the new/additional behavior for the ternary operator or other bool-testing assignment situations (eg: "void f(bool); f(x = 5);")
>>
>> On Mon, May 18, 2020 at 6:53 AM Billy Araujo via cfe-dev <[hidden email]> wrote:
>>>
>>> -Wparentheses
>>>
>>> warns for line 3 but not for the ternary expression in line 2.
>>>
>>> 1 static void foo(int a, int b, int x) {
>>> 2    x = (x = 10) ? a : b;
>>> 3    if (x = 10) { x = a; } else { x = b; }
>>> 4 }
>>>
>>> Is this a bug?
This seems pretty correct to me.

You can't avoid braces on line 3, so those braces don't count as extra braces
to silence the diagnostic, while the situation on the line 2 is opposite,
no braces are needed there, so the braces count as silencers.

I.e. if line 2 *would* be diagnosed, i'd say that is a false-positive.

>>> Regards,
>>>
>>> Billy.
Roman

>>> _______________________________________________
>>> cfe-dev mailing list
>>> [hidden email]
>>> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>
> _______________________________________________
> cfe-dev mailing list
> [hidden email]
> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: parentheses flag warning

Fangrui Song via cfe-dev

Hi David,

Actually the situation you present foo(x =5); is interesting because gcc 10 flags this while clang 10 doesn't: 


warning: suggest parentheses around assignment used as truth value 

@Roman. Thanks for the further explanation. That makes sense.

Regards,

Billy.



On Mon, May 18, 2020 at 6:49 PM Roman Lebedev <[hidden email]> wrote:
On Mon, May 18, 2020 at 8:44 PM Billy Araujo via cfe-dev
<[hidden email]> wrote:
>
> Hi David,
>
> So I guess this is expected behaviour. Thanks.
>
> Regards,
>
> Billy.
>
>
> On Mon, May 18, 2020 at 6:39 PM David Blaikie <[hidden email]> wrote:
>>
>> GCC doesn't seem to warn on this either. On the basis of the original warning I could see how this case could merit similar handling - but I doubt we'd want to add that handling to the existing flag at this point (due to the number of people over the decades that have used the warning for the behavior it currently provides, not expecting the new/additional behavior for the ternary operator or other bool-testing assignment situations (eg: "void f(bool); f(x = 5);")
>>
>> On Mon, May 18, 2020 at 6:53 AM Billy Araujo via cfe-dev <[hidden email]> wrote:
>>>
>>> -Wparentheses
>>>
>>> warns for line 3 but not for the ternary expression in line 2.
>>>
>>> 1 static void foo(int a, int b, int x) {
>>> 2    x = (x = 10) ? a : b;
>>> 3    if (x = 10) { x = a; } else { x = b; }
>>> 4 }
>>>
>>> Is this a bug?
This seems pretty correct to me.

You can't avoid braces on line 3, so those braces don't count as extra braces
to silence the diagnostic, while the situation on the line 2 is opposite,
no braces are needed there, so the braces count as silencers.

I.e. if line 2 *would* be diagnosed, i'd say that is a false-positive.

>>> Regards,
>>>
>>> Billy.
Roman

>>> _______________________________________________
>>> cfe-dev mailing list
>>> [hidden email]
>>> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>
> _______________________________________________
> cfe-dev mailing list
> [hidden email]
> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: parentheses flag warning

Fangrui Song via cfe-dev
In reply to this post by Fangrui Song via cfe-dev


On Mon, May 18, 2020 at 10:49 AM Roman Lebedev <[hidden email]> wrote:
On Mon, May 18, 2020 at 8:44 PM Billy Araujo via cfe-dev
<[hidden email]> wrote:
>
> Hi David,
>
> So I guess this is expected behaviour. Thanks.
>
> Regards,
>
> Billy.
>
>
> On Mon, May 18, 2020 at 6:39 PM David Blaikie <[hidden email]> wrote:
>>
>> GCC doesn't seem to warn on this either. On the basis of the original warning I could see how this case could merit similar handling - but I doubt we'd want to add that handling to the existing flag at this point (due to the number of people over the decades that have used the warning for the behavior it currently provides, not expecting the new/additional behavior for the ternary operator or other bool-testing assignment situations (eg: "void f(bool); f(x = 5);")
>>
>> On Mon, May 18, 2020 at 6:53 AM Billy Araujo via cfe-dev <[hidden email]> wrote:
>>>
>>> -Wparentheses
>>>
>>> warns for line 3 but not for the ternary expression in line 2.
>>>
>>> 1 static void foo(int a, int b, int x) {
>>> 2    x = (x = 10) ? a : b;
>>> 3    if (x = 10) { x = a; } else { x = b; }
>>> 4 }
>>>
>>> Is this a bug?
This seems pretty correct to me.

You can't avoid braces on line 3, so those braces don't count as extra braces
to silence the diagnostic, while the situation on the line 2 is opposite,
no braces are needed there, so the braces count as silencers.

But braces are needed there, I think, otherwise precedence makes "i  = 3 ? a : b" into "i = (3 ? a : b)", right?
 

I.e. if line 2 *would* be diagnosed, i'd say that is a false-positive.

>>> Regards,
>>>
>>> Billy.
Roman

>>> _______________________________________________
>>> cfe-dev mailing list
>>> [hidden email]
>>> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>
> _______________________________________________
> cfe-dev mailing list
> [hidden email]
> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: parentheses flag warning

Fangrui Song via cfe-dev
In reply to this post by Fangrui Song via cfe-dev


On Mon, May 18, 2020 at 10:59 AM Billy Araujo <[hidden email]> wrote:

Hi David,

Actually the situation you present foo(x =5); is interesting because gcc 10 flags this while clang 10 doesn't: 


warning: suggest parentheses around assignment used as truth value 


Ah, well, missed opportunity for Clang, then! (bug/missing feature, I'd say - though clang-to-clang backwards compatibility might make that hard to roll out in clang at this point, not sure)
 
@Roman. Thanks for the further explanation. That makes sense.

Regards,

Billy.



On Mon, May 18, 2020 at 6:49 PM Roman Lebedev <[hidden email]> wrote:
On Mon, May 18, 2020 at 8:44 PM Billy Araujo via cfe-dev
<[hidden email]> wrote:
>
> Hi David,
>
> So I guess this is expected behaviour. Thanks.
>
> Regards,
>
> Billy.
>
>
> On Mon, May 18, 2020 at 6:39 PM David Blaikie <[hidden email]> wrote:
>>
>> GCC doesn't seem to warn on this either. On the basis of the original warning I could see how this case could merit similar handling - but I doubt we'd want to add that handling to the existing flag at this point (due to the number of people over the decades that have used the warning for the behavior it currently provides, not expecting the new/additional behavior for the ternary operator or other bool-testing assignment situations (eg: "void f(bool); f(x = 5);")
>>
>> On Mon, May 18, 2020 at 6:53 AM Billy Araujo via cfe-dev <[hidden email]> wrote:
>>>
>>> -Wparentheses
>>>
>>> warns for line 3 but not for the ternary expression in line 2.
>>>
>>> 1 static void foo(int a, int b, int x) {
>>> 2    x = (x = 10) ? a : b;
>>> 3    if (x = 10) { x = a; } else { x = b; }
>>> 4 }
>>>
>>> Is this a bug?
This seems pretty correct to me.

You can't avoid braces on line 3, so those braces don't count as extra braces
to silence the diagnostic, while the situation on the line 2 is opposite,
no braces are needed there, so the braces count as silencers.

I.e. if line 2 *would* be diagnosed, i'd say that is a false-positive.

>>> Regards,
>>>
>>> Billy.
Roman

>>> _______________________________________________
>>> cfe-dev mailing list
>>> [hidden email]
>>> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>
> _______________________________________________
> cfe-dev mailing list
> [hidden email]
> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: parentheses flag warning

Fangrui Song via cfe-dev
In reply to this post by Fangrui Song via cfe-dev
On Mon, May 18, 2020 at 1:50 PM Roman Lebedev via cfe-dev <[hidden email]> wrote:
> On Mon, May 18, 2020 at 6:39 PM David Blaikie <[hidden email]> wrote:
>> On Mon, May 18, 2020 at 6:53 AM Billy Araujo via cfe-dev <[hidden email]> wrote:
>>>
>>> -Wparentheses warns for line 3 but not for the ternary expression in line 2.
>>>
>>> 1 static void foo(int a, int b, int x) {
>>> 2    x = (x = 10) ? a : b;
>>> 3    if (x = 10) { x = a; } else { x = b; }
>>> 4 }
>>>
>>> Is this a bug?
This seems pretty correct to me.

You can't avoid braces on line 3, so those braces don't count as extra braces
to silence the diagnostic, while the situation on the line 2 is opposite,
no braces are needed there, so the braces count as silencers.

I.e. if line 2 *would* be diagnosed, i'd say that is a false-positive.

Besides what David subsequently pointed out — that the parens in `(x = 10) ? a : b` do affect the precedence — I believe you're looking at -Wparentheses in the wrong light. The criterion for avoiding false positives is not that it should never warn about any number of parens more than the physically minimal amount. The criterion is that it should not warn if the number of parens is high enough to indicate that the user is aware of the issue and actively trying to suppress the warning.

For example,
    if ((x < 5) || (x = 7)) ;
should warn, because it is "quite obvious" that the user meant (x == 7), not (x = 7).  How do we know?  Well, we imagine that the user had written `==` instead of `=`, and we look to see whether the expression would have been idiomatic.
    if ((x = 7)) ;
historically does not warn, and by this criterion should not warn. Why? Well, because if we imagine that the user had written `==` instead of `=`, we'd get
    if ((x == 7)) ;
which has an unidiomatic extra pair of parens. This indicates that the user must be going out of their way to suppress the diagnostic.

Neither GCC nor Clang currently warn on `((x < 5) || (x = 7))`. I guarantee that as soon as you start warning about it, you'll uncover real bugs in real codebases.

–Arthur

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev