This is an archive of the discontinued LLVM Phabricator instance.

[clang][Sema]Print diagnostic warning about precedence when integer expression is used without parentheses in an conditional operator expression
Needs ReviewPublic

Authored by chaitanyav on Apr 8 2023, 1:50 AM.

Details

Reviewers
aaron.ballman
Mordante
dblaikie
cjdb
echristo
jdoerfert
Group Reviewers
Restricted Project
Restricted Project
Restricted Project
Summary

Fixes issue https://github.com/llvm/llvm-project/issues/61943

#include <iostream>

int main()
{
    int ai32 = 5;
    std::cout << true ? 1 : 0;
    std::cout << "Test" << ai32 ? 1 : 0;
    int b = ai32 & 1 ? true : false;
    auto c = true ? "1" : "0";
    return 0;
}

Before the change

test.cpp:6:23: warning: operator '?:' has lower precedence than '<<'; '<<' will be evaluated first [-Wparentheses]
    std::cout << true ? 1 : 0;
    ~~~~~~~~~~~~~~~~~ ^
test.cpp:6:23: note: place parentheses around the '<<' expression to silence this warning
    std::cout << true ? 1 : 0;
                      ^
    (                )
test.cpp:6:23: note: place parentheses around the '?:' expression to evaluate it first
    std::cout << true ? 1 : 0;
                      ^
                 (           )
1 warning generated.

After the change

test.cpp:6:23: warning: operator '?:' has lower precedence than '<<'; '<<' will be evaluated first [-Wparentheses]
    std::cout << true ? 1 : 0;
    ~~~~~~~~~~~~~~~~~ ^
test.cpp:6:23: note: place parentheses around the '<<' expression to silence this warning
    std::cout << true ? 1 : 0;
                      ^
    (                )
test.cpp:6:23: note: place parentheses around the '?:' expression to evaluate it first
    std::cout << true ? 1 : 0;
                      ^
                 (           )
test.cpp:7:33: warning: operator '?:' has lower precedence than '<<'; '<<' will be evaluated first [-Wparentheses]
    std::cout << "Test" << ai32 ? 1 : 0;
    ~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^
test.cpp:7:33: note: place parentheses around the '<<' expression to silence this warning
    std::cout << "Test" << ai32 ? 1 : 0;
                                ^
    (                          )
test.cpp:7:33: note: place parentheses around the '?:' expression to evaluate it first
    std::cout << "Test" << ai32 ? 1 : 0;
                                ^
                           (           )
test.cpp:8:22: warning: operator '?:' has lower precedence than '&'; '&' will be evaluated first [-Wbitwise-conditional-parentheses]
    int b = ai32 & 1 ? true : false;
            ~~~~~~~~ ^
test.cpp:8:22: note: place parentheses around the '&' expression to silence this warning
    int b = ai32 & 1 ? true : false;
                     ^
            (       )
test.cpp:8:22: note: place parentheses around the '?:' expression to evaluate it first
    int b = ai32 & 1 ? true : false;
                     ^
                   (               )
3 warnings generated.

Diff Detail

Event Timeline

chaitanyav created this revision.Apr 8 2023, 1:50 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 8 2023, 1:50 AM
chaitanyav requested review of this revision.Apr 8 2023, 1:50 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 8 2023, 1:50 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

output for testcases mentioned in https://github.com/llvm/llvm-project/issues/61943

test.cpp:6:30: warning: overloaded operator << has higher precedence than comparison operator [-Woverloaded-shift-op-parentheses]
    std::cout << "Test" << a == 5 ? 1 : 0;
    ~~~~~~~~~~~~~~~~~~~~~~~~ ^  ~
test.cpp:6:25: note: place parentheses around the '<<' expression to silence this warning
    std::cout << "Test" << a == 5 ? 1 : 0;
                        ^
    (                       )
test.cpp:6:30: note: place parentheses around comparison expression to evaluate it first
    std::cout << "Test" << a == 5 ? 1 : 0;
                             ^
                           (     )
test.cpp:6:30: error: invalid operands to binary expression ('ostream' and 'int')
    std::cout << "Test" << a == 5 ? 1 : 0;
    ~~~~~~~~~~~~~~~~~~~~~~~~ ^  ~
test.cpp:7:30: warning: operator '?:' has lower precedence than '<<'; '<<' will be evaluated first [-Wparentheses]
    std::cout << "Test" << a ? 1 : 0;
    ~~~~~~~~~~~~~~~~~~~~~~~~ ^
test.cpp:7:30: note: place parentheses around the '<<' expression to silence this warning
    std::cout << "Test" << a ? 1 : 0;
                             ^
    (                       )
test.cpp:7:30: note: place parentheses around the '?:' expression to evaluate it first
    std::cout << "Test" << a ? 1 : 0;
                             ^
                           (        )
test.cpp:8:49: warning: operator '?:' has lower precedence than '<<'; '<<' will be evaluated first [-Wparentheses]
    std::cout << "Test" << static_cast<bool>(a) ? 1 : 0;
    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^
test.cpp:8:49: note: place parentheses around the '<<' expression to silence this warning
    std::cout << "Test" << static_cast<bool>(a) ? 1 : 0;
                                                ^
    (                                          )
test.cpp:8:49: note: place parentheses around the '?:' expression to evaluate it first
    std::cout << "Test" << static_cast<bool>(a) ? 1 : 0;
                                                ^

@aaron.ballman Please review

chaitanyav abandoned this revision.Apr 14 2023, 2:25 PM
chaitanyav reclaimed this revision.Apr 17 2023, 7:17 AM

re-opening the revision to make further updates as per comments on the github issue.

Thank you for working on this! The only thing I think is missing is a release note, though I did have a question on a change in the tests.

clang/test/Sema/parentheses.cpp
34

Is this change necessary? Alternatively, can we add this as an overload instead of changing the operator?

Update release notes about the fix

aaron.ballman added inline comments.Apr 18 2023, 11:53 AM
clang/test/Sema/parentheses.cpp
34

I'm still wondering about this.

chaitanyav added a comment.EditedApr 18 2023, 12:45 PM

@aaron.ballman am looking into this. If i change it back to
operation int();
and add
Stream& operator<<(bool)
i get
`
error: 'warning' diagnostics expected but not seen:

File /usr/home/nvellanki/explore/llvm-project/clang/test/Sema/parentheses.cpp Line 68: operator '?:' has lower precedence than '<<'

error: 'note' diagnostics expected but not seen:

File /usr/home/nvellanki/explore/llvm-project/clang/test/Sema/parentheses.cpp Line 68: place parentheses around the '<<' expression to silence this warning
File /usr/home/nvellanki/explore/llvm-project/clang/test/Sema/parentheses.cpp Line 68: place parentheses around the '?:' expression to evaluate it first

3 errors generated.
`
But it works correctly with std::cout

`
test.cpp:7:30: warning: operator '?:' has lower precedence than '<<'; '<<' will be evaluated first [-Wparentheses]

std::cout << "Test" << a ? 1 : 0;
~~~~~~~~~~~~~~~~~~~~~~~~ ^

test.cpp:7:30: note: place parentheses around the '<<' expression to silence this warning

std::cout << "Test" << a ? 1 : 0;
                         ^
(                       )

test.cpp:7:30: note: place parentheses around the '?:' expression to evaluate it first

std::cout << "Test" << a ? 1 : 0;
                         ^
                       (        )

@aaron.ballman am looking into this. If i change it back to
operation int();
and add
Stream& operator<<(bool)
i get
`
error: 'warning' diagnostics expected but not seen:

File /usr/home/nvellanki/explore/llvm-project/clang/test/Sema/parentheses.cpp Line 68: operator '?:' has lower precedence than '<<'

error: 'note' diagnostics expected but not seen:

File /usr/home/nvellanki/explore/llvm-project/clang/test/Sema/parentheses.cpp Line 68: place parentheses around the '<<' expression to silence this warning
File /usr/home/nvellanki/explore/llvm-project/clang/test/Sema/parentheses.cpp Line 68: place parentheses around the '?:' expression to evaluate it first

3 errors generated.
`
But it works correctly with std::cout

`
test.cpp:7:30: warning: operator '?:' has lower precedence than '<<'; '<<' will be evaluated first [-Wparentheses]

std::cout << "Test" << a ? 1 : 0;
~~~~~~~~~~~~~~~~~~~~~~~~ ^

test.cpp:7:30: note: place parentheses around the '<<' expression to silence this warning

std::cout << "Test" << a ? 1 : 0;
                         ^
(                       )

test.cpp:7:30: note: place parentheses around the '?:' expression to evaluate it first

std::cout << "Test" << a ? 1 : 0;
                         ^
                       (        )

I would expect to get the same diagnostic with either operator int() or operator bool() as the precedence is the same either way. I think what might be happening here is that ExprLooksBoolean is getting tricked into thinking the expression isn't boolean when it actually is (I think this might be the FIXME comment in that function needing to be addressed).

@aaron.ballman am looking into this. If i change it back to
operation int();
and add
Stream& operator<<(bool)
i get
`
error: 'warning' diagnostics expected but not seen:

File /usr/home/nvellanki/explore/llvm-project/clang/test/Sema/parentheses.cpp Line 68: operator '?:' has lower precedence than '<<'

error: 'note' diagnostics expected but not seen:

File /usr/home/nvellanki/explore/llvm-project/clang/test/Sema/parentheses.cpp Line 68: place parentheses around the '<<' expression to silence this warning
File /usr/home/nvellanki/explore/llvm-project/clang/test/Sema/parentheses.cpp Line 68: place parentheses around the '?:' expression to evaluate it first

3 errors generated.
`
But it works correctly with std::cout

`
test.cpp:7:30: warning: operator '?:' has lower precedence than '<<'; '<<' will be evaluated first [-Wparentheses]

std::cout << "Test" << a ? 1 : 0;
~~~~~~~~~~~~~~~~~~~~~~~~ ^

test.cpp:7:30: note: place parentheses around the '<<' expression to silence this warning

std::cout << "Test" << a ? 1 : 0;
                         ^
(                       )

test.cpp:7:30: note: place parentheses around the '?:' expression to evaluate it first

std::cout << "Test" << a ? 1 : 0;
                         ^
                       (        )

I would expect to get the same diagnostic with either operator int() or operator bool() as the precedence is the same either way. I think what might be happening here is that ExprLooksBoolean is getting tricked into thinking the expression isn't boolean when it actually is (I think this might be the FIXME comment in that function needing to be addressed).

Thanks @aaron.ballman, will look into the FIXME.

Emit warning when implicit cast from int to bool in an conditional operator expression

chaitanyav marked 2 inline comments as done.Apr 19 2023, 4:41 PM

@aaron.ballman I have made more changes so that the warning is emitted on other integer expressions where the opertors has higher precedence than conditional operator.

Update SemaCXX tests to use parentheses around conditional operator

Update test to use parentheses around conditional operator

chaitanyav updated this revision to Diff 515445.EditedApr 20 2023, 12:57 PM

Place parentheses around & expression since -Werror is enabled for build

:/home/libcxx-builder/.buildkite-agent/builds/google-libcxx-builder-libcxx-precommit-ci-group-60nx-1/llvm-project/libcxx-ci/build/generic-cxx2b/include/c++/v1/__chrono/duration.h:197:32: error: operator '?:' has lower precedence than '&'; '&' will be evaluated first [-Werror,-Wbitwise-conditional-parentheses]
2650-    return __lower.count() & 1 ? __upper : __lower;
2651-           ~~~~~~~~~~~~~~~~~~~ ^
2652:/home/libcxx-builder/.buildkite-agent/builds/google-libcxx-builder-libcxx-precommit-ci-group-60nx-1/llvm-project/libcxx-ci/build/generic-cxx2b/include/c++/v1/__chrono/duration.h:197:32: note: place parentheses around the '&' expression to silence this warning
2653-    return __lower.count() & 1 ? __upper : __lower;
2654-                               ^
2655-           (                  )
2656:/home/libcxx-builder/.buildkite-agent/builds/google-libcxx-builder-libcxx-precommit-ci-group-60nx-1/llvm-project/libcxx-ci/build/generic-cxx2b/include/c++/v1/__chrono/duration.h:197:32: note: place parentheses around the '?:' expression to evaluate it first
2657-    return __lower.count() & 1 ? __upper : __lower;
2658-                               ^
2659-                             (                    )
2660-1 error generated.
/home/libcxx-builder/.buildkite-agent/builds/google-libcxx-builder-libcxx-precommit-ci-group-60nx-1/llvm-project/libcxx-ci/libcxxabi/src/cxa_personality.cpp:722:38: error: operator '?:' has lower precedence than '&'; '&' will be evaluated first [-Werror,-Wbitwise-conditional-parentheses]
                                     ? _URC_CONTINUE_UNWIND
                                     ^
/home/libcxx-builder/.buildkite-agent/builds/google-libcxx-builder-libcxx-precommit-ci-group-60nx-1/llvm-project/libcxx-ci/libcxxabi/src/cxa_personality.cpp:722:38: note: place parentheses around the '&' expression to silence this warning
                                     ? _URC_CONTINUE_UNWIND
                                     ^
/home/libcxx-builder/.buildkite-agent/builds/google-libcxx-builder-libcxx-precommit-ci-group-60nx-1/llvm-project/libcxx-ci/libcxxabi/src/cxa_personality.cpp:722:38: note: place parentheses around the '?:' expression to evaluate it first
                                     ? _URC_CONTINUE_UNWIND
                                     ^
1 error generated.
Herald added a project: Restricted Project. · View Herald TranscriptApr 20 2023, 12:57 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
chaitanyav retitled this revision from Emit warning when implicit cast to bool happens in an conditional operator expression when used inside an overloaded shift operator expression to Emit warning when implicit cast from int to bool happens in an conditional operator expression.Apr 20 2023, 1:02 PM

Place parentheses around conditional expression to prevent precedence warnings since -Werror is enabled on build

Herald added a project: Restricted Project. · View Herald TranscriptApr 20 2023, 2:41 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript

clang-format the changes to the file

Place parentheses around conditional expression to prevent precedence warnings since -Werror is enabled on build

chaitanyav added a comment.EditedApr 21 2023, 1:12 AM

@aaron.ballman please take a look.

I filed a issue that premerge checks are not working on AIX
https://github.com/google/llvm-premerge-checks/issues/441

Not sure why the TSAN, MSAN and ASAN checks are failing on libcxx
https://buildkite.com/llvm-project/libcxx-ci/builds/22664

I have to say I'm not really convinced this change is a good idea. The cases it flags don't really seem in any way ambiguous/erroneous.

@aaron.ballman please take a look.

I filed a issue that premerge checks are not working on AIX
https://github.com/google/llvm-premerge-checks/issues/441

Not sure why the TSAN, MSAN and ASAN checks are failing on libcxx
https://buildkite.com/llvm-project/libcxx-ci/builds/22664

The AIX builders are currently broken. That has to do with an update. We don't really know what's going on with the sanitizer builds, but these are unrelated too (probably also a problem with the builders).

libcxx/include/strstream
303

Please don't change the indentation. It was correct before.

Fix indentation

Mordante accepted this revision.Apr 21 2023, 10:21 AM
Mordante added a subscriber: Mordante.

I too am not really convinced by all changes in libc++. On the other hand I don't think the change make the libc++ code looking bad. So I don't feel a reason to reject the libc++ changes.

I don't like the commit message, there's only a title and a bug report. Please add a bit more information in the message, with an example of code that now gives a diagnostic.

Note I only reviewed the libc++ changes, I leave the clang part to @aaron.ballman.

chaitanyav edited the summary of this revision. (Show Details)Apr 21 2023, 1:31 PM
chaitanyav marked an inline comment as done.

Modify the commit message

chaitanyav retitled this revision from Emit warning when implicit cast from int to bool happens in an conditional operator expression to [clang][Sema]Print diagnostic warning when implicit cast from int to bool happens in an conditional operator expression.Apr 21 2023, 6:40 PM

Revise commit message

chaitanyav retitled this revision from [clang][Sema]Print diagnostic warning when implicit cast from int to bool happens in an conditional operator expression to [clang][Sema]Print diagnostic warning about precedence when integer expression is used without parentheses in an conditional operator expression.Apr 21 2023, 7:39 PM

One suggestion for the commit message either use [[maybe_unused]] or -Wno-unused that way the unrelated messages aren't shown in the commit message.

chaitanyav edited the summary of this revision. (Show Details)Apr 22 2023, 8:46 AM
aaron.ballman added reviewers: dblaikie, cjdb, echristo, Restricted Project.Apr 24 2023, 11:42 AM

I have to say I'm not really convinced this change is a good idea. The cases it flags don't really seem in any way ambiguous/erroneous.

I think some of the cases are ambiguous while others are not. It's taken me a while to come up with what I think my brain is doing, maybe this happens for others as well. When the ternary conditional expression is a binary expression, my brain can figure things out very quickly when the RHS of that binary expression is a literal but my brain is much less confident when the RHS of that binary expression is an identifier. e.g., x & 1 ? foo : bar is easy for my brain to go "well, obviously the condition is not testing whether 1 is nonzero, so it must be testing the result of x & 1 instead", but something like x & y ? foo : bar is far less obvious as to what the behavior is because x & (y ? foo : bar) is as reasonable a choice as (x & y) ? foo : bar without putting a lot more thought into it. However, that might be splitting hairs with the diagnostic functionality (esp because macros and enumerations are sort of like literals and sort of like identifiers, depending on the way you want to think of them).

Adding in some more folks for opinions on whether the proposed changes in this patch are an improvement or whether we want to tweak the heuristic a bit more. My original thinking was that all precedence situations with ternary conditional expressions are kind of equally confusing, but given that two folks have pushed back on the proposed changes, more opinions would be valuable.

cjdb added a comment.Apr 24 2023, 12:27 PM

I think this is a good diagnostic to add: it improves readability and eliminates ambiguities. My only request is that if there isn't already a FixIt hint, one be added, please.

clang/test/Sema/parentheses.c
94

I think this should also warn.

I think some of the cases are ambiguous while others are not.

Data would be good to have - if this assessment is true, we'd expect this to bear out in terms of bug finding, yeah? (that the cases you find to be ambiguous turn up as real bugs with some significant frequency in a codebase?)

I'm afraid I have been bitten by this very issue several more than once. I can only assume I'm not the only one. so this looks like a nice improvement.
There are only a few changes in libc++ so it seems fairly low noise.

Did you try to compile llvm with it? I'd be curious to see how many time these occurs in clang own diagnostics.

cjdb added a comment.Apr 24 2023, 2:25 PM

I think some of the cases are ambiguous while others are not.

Data would be good to have - if this assessment is true, we'd expect this to bear out in terms of bug finding, yeah? (that the cases you find to be ambiguous turn up as real bugs with some significant frequency in a codebase?)

I disagree that there's a need for bugs to add this warning. Reading ambiguous-but-correct code isn't going to be buggy, but it is going to cause readability issues for any reviewers or maintainers.

I think some of the cases are ambiguous while others are not.

Data would be good to have - if this assessment is true, we'd expect this to bear out in terms of bug finding, yeah? (that the cases you find to be ambiguous turn up as real bugs with some significant frequency in a codebase?)

I disagree that there's a need for bugs to add this warning. Reading ambiguous-but-correct code isn't going to be buggy, but it is going to cause readability issues for any reviewers or maintainers.

That's generally been the bar we use for evaluating warnings - does it find bugs. Especially because if it doesn't, it's unlikely to be turned on on large pre-existing codebases owing to the cost of cleaning them up with limited value in terms of improving readability but not finding any bugs. (& goes hand-in-hand with the general policy of not adding off-by-default warnings, because they don't get used much and come at a cost to clang's codebase (& some (death-by-a-thousand-cuts) cost to compile time performance, even when the warning is disabled))

Readability improvements that don't cross the threshold to be the cause of a significant number of bugs are moreso the domain of clang-tidy, not clang warnings.

I think this is a good diagnostic to add: it improves readability and eliminates ambiguities. My only request is that if there isn't already a FixIt hint, one be added, please.

@cjb quick question, should we restrict to

>, <, >=, <=

I am finding lots of files in the project with == and ?: precedence warnings.

clang/test/Sema/parentheses.c
94

@cjdb am looking into this.

Fix tests/code by adding parentheses around the conditional operator expression

Added parentheses to lots of files to fix the precedence warning. Still a lot to change in OpenMP.

Fix more failing tests due to missing parentheses in conditional operator expression

I think some of the cases are ambiguous while others are not.

Data would be good to have - if this assessment is true, we'd expect this to bear out in terms of bug finding, yeah? (that the cases you find to be ambiguous turn up as real bugs with some significant frequency in a codebase?)

I disagree that there's a need for bugs to add this warning. Reading ambiguous-but-correct code isn't going to be buggy, but it is going to cause readability issues for any reviewers or maintainers.

The number of changes to test cases convinces me that we definitely need data on whether it's worth it or not. This warning now looks chatty enough that it needs to be disabled by default, and that doesn't meet our usual bar.

Readability improvements that don't cross the threshold to be the cause of a significant number of bugs are moreso the domain of clang-tidy, not clang warnings.

Strong +1, this is starting to feel more like a style warning due to the chattiness.

cjdb added a comment.Apr 26 2023, 9:59 AM

I think some of the cases are ambiguous while others are not.

Data would be good to have - if this assessment is true, we'd expect this to bear out in terms of bug finding, yeah? (that the cases you find to be ambiguous turn up as real bugs with some significant frequency in a codebase?)

I disagree that there's a need for bugs to add this warning. Reading ambiguous-but-correct code isn't going to be buggy, but it is going to cause readability issues for any reviewers or maintainers.

That's generally been the bar we use for evaluating warnings - does it find bugs. Especially because if it doesn't, it's unlikely to be turned on on large pre-existing codebases owing to the cost of cleaning them up with limited value in terms of improving readability but not finding any bugs. (& goes hand-in-hand with the general policy of not adding off-by-default warnings, because they don't get used much and come at a cost to clang's codebase (& some (death-by-a-thousand-cuts) cost to compile time performance, even when the warning is disabled))

Readability improvements that don't cross the threshold to be the cause of a significant number of bugs are moreso the domain of clang-tidy, not clang warnings.

Fair enough, I often mix up which project a warning belongs to, so thanks for the reminder :)

@aaron.ballman just for learning, can you point me to the code where a warning is selectively enabled/disabled based on a flag. For e.g. -Wunused-comparison

@aaron.ballman just for learning, can you point me to the code where a warning is selectively enabled/disabled based on a flag. For e.g. -Wunused-comparison

Sure! This is handled automagically for you by tablegen when you use DefaultIgnore while defining the diagnostic, as in: https://github.com/llvm/llvm-project/blob/c4505158950f2211d8c4b429049f2cd765f3e092/clang/include/clang/Basic/DiagnosticSemaKinds.td#L28

Disable precedence conditional warning by default; Revert changes to test files

Disable precedence conditional warning by default; Revert changes to test files

I'm sorry, I think there was a miscommunication. We were saying that we don't typically want off-by-default diagnostics at all, not asking to turn this one off by default. We've found that when a diagnostic is off by default, it very rarely gets enabled by enough projects to be worth having the diagnostic. So we instead expect diagnostics to have a very low false positive rate so that they can be on by default without worrying that a lot of users will have to disable it due to chattiness.

So instead of making this diagnostic be DefaultIgnore, I think you should back out most of the recent changes so we're back to only the small number of additional diagnostics in the earlier patches.

Disable precedence conditional warning by default; Revert changes to test files

I'm sorry, I think there was a miscommunication. We were saying that we don't typically want off-by-default diagnostics at all, not asking to turn this one off by default. We've found that when a diagnostic is off by default, it very rarely gets enabled by enough projects to be worth having the diagnostic. So we instead expect diagnostics to have a very low false positive rate so that they can be on by default without worrying that a lot of users will have to disable it due to chattiness.

So instead of making this diagnostic be DefaultIgnore, I think you should back out most of the recent changes so we're back to only the small number of additional diagnostics in the earlier patches.

@aaron.ballman am reverting to the earliest patch.

Revert to earliest patch

Add parentheses around integer expressions in libcxx tests

Revert libcxx libcxxabi files unrelated to the issue

Rebase with upstream

chaitanyav updated this revision to Diff 519259.May 3 2023, 2:44 PM

Rebase with upstream

chaitanyav updated this revision to Diff 520491.May 8 2023, 3:31 PM

Rebase with upstream

In general, I think this is incremental progress on the diagnostic behavior. However, it's clear that there is room for interpretation on what is or is not a false positive diagnostic for this, so we should pay close attention to user feedback during the 17.x release cycle. If it seems we're getting significant push back, we may need to come back and rethink.

Specifically, I think we've improved the situation for code like this:

// `p` is a pointer, `x` is an `int`, and `b` is a `bool`
p + x ? 1 : 2; // previously didn't warn, now warns
p + b ? 1 : 2; // always warned

Does anyone feel we should not move forward with accepting the patch in its current form?

clang/docs/ReleaseNotes.rst
374–375

It looks like a rebase accidentally duplicated this line.

chaitanyav updated this revision to Diff 520725.May 9 2023, 9:19 AM

Rebase with upstream and remove duplicate line from ReleaseNotes

ldionne accepted this revision as: Restricted Project.May 10 2023, 2:47 PM
ldionne added a subscriber: ldionne.

Code changes in libc++ and libc++abi LGTM. I am neutral on whether the diagnostic is worth adding, but don't consider libc++ and libc++abi as blockers for this patch.

libcxxabi/src/cxa_personality.cpp
721

Double-parens are not required.

This revision is now accepted and ready to land.May 10 2023, 2:47 PM

Remove extra parens

Rebase with upstream

Code changes in libc++ and libc++abi LGTM. I am neutral on whether the diagnostic is worth adding, but don't consider libc++ and libc++abi as blockers for this patch.

Thank you @ldionne! My plan is to accept & land this if we hear no objections by next Thur (May 18).

In general, I think this is incremental progress on the diagnostic behavior. However, it's clear that there is room for interpretation on what is or is not a false positive diagnostic for this,

I hope we can agree on what a false positive is here - when the warning fires but the code is what the developer intended (ie: the existing code with the existing language semantics produce the desired result, the "fix" is to add parentheses that explicitly encode the language's existing rules/behavior anyway).

Not that we don't have warnings that do this - that encourage parens to reinforce what the language already does to be more explicit/intentional about it, and in some cases it's not that uncommon that the user will be adding parens that reinforce the precedence rules anyway.

Like, I think all the fixes in libc++, llvm, etc, are false positives? (none of them found bugs/unintended behavior) Are there any examples of bugs being found by this warning in a codebase? (& how many false positives in such a codebase did it also flag?)

so we should pay close attention to user feedback during the 17.x release cycle. If it seems we're getting significant push back, we may need to come back and rethink.

Specifically, I think we've improved the situation for code like this:

// `p` is a pointer, `x` is an `int`, and `b` is a `bool`
p + x ? 1 : 2; // previously didn't warn, now warns
p + b ? 1 : 2; // always warned

Does anyone feel we should not move forward with accepting the patch in its current form?

*goes to look*

Mixed feelings - you're right, incremental improvement/adding missed cases to an existing warning (especially if that warning's already stylistic in nature) is a lower bar/doesn't necessarily need the false positive assessment. But it looks like this case might've been intentionally suppressed in the initial warning implementation? (anyone linked to the original warning implementation/review/design to check if this was intentional?)

But, yeah, seems marginal enough I don't have strong feelings either way.

Rebase with upstream

Rebase with upstream

Rebase with upstream

In general, I think this is incremental progress on the diagnostic behavior. However, it's clear that there is room for interpretation on what is or is not a false positive diagnostic for this,

I hope we can agree on what a false positive is here - when the warning fires but the code is what the developer intended (ie: the existing code with the existing language semantics produce the desired result, the "fix" is to add parentheses that explicitly encode the language's existing rules/behavior anyway).

I agree with that definition -- that's a useful way to approach this, thank you!

Not that we don't have warnings that do this - that encourage parens to reinforce what the language already does to be more explicit/intentional about it, and in some cases it's not that uncommon that the user will be adding parens that reinforce the precedence rules anyway.

Yup, which is largely what this patch is about.

Like, I think all the fixes in libc++, llvm, etc, are false positives? (none of them found bugs/unintended behavior)

Yes, they all are false positives by the above definition.

Are there any examples of bugs being found by this warning in a codebase? (& how many false positives in such a codebase did it also flag?)

This would be good to know, but a bit of a heavy lift to require of @chaitanyav because they were working on this issue (https://github.com/llvm/llvm-project/issues/61943) with a "good first issue" label that is starting to look a bit like it was misleading (sorry about that!). However, if you're able to try compiling some larger projects with your patch applied to see if it spots any bugs in real world code, that would be very helpful!

so we should pay close attention to user feedback during the 17.x release cycle. If it seems we're getting significant push back, we may need to come back and rethink.

Specifically, I think we've improved the situation for code like this:

// `p` is a pointer, `x` is an `int`, and `b` is a `bool`
p + x ? 1 : 2; // previously didn't warn, now warns
p + b ? 1 : 2; // always warned

Does anyone feel we should not move forward with accepting the patch in its current form?

*goes to look*

Mixed feelings - you're right, incremental improvement/adding missed cases to an existing warning (especially if that warning's already stylistic in nature) is a lower bar/doesn't necessarily need the false positive assessment. But it looks like this case might've been intentionally suppressed in the initial warning implementation? (anyone linked to the original warning implementation/review/design to check if this was intentional?)

I tried to chase down where this got added to see what review comments there were, but it seems to predate my involvement (I'm seeing mentions of this in 2011).

But, yeah, seems marginal enough I don't have strong feelings either way.

Thank you for the opinion! I think pointer + int is a far more common code pattern than pointer + bool, so it makes some sense to me that we would silence the first case while diagnosing the second case. Given the general lack of enthusiasm for the new diagnostics, it may boil down to answering whether this finds any true positives or not.

In general, I think this is incremental progress on the diagnostic behavior. However, it's clear that there is room for interpretation on what is or is not a false positive diagnostic for this,

I hope we can agree on what a false positive is here - when the warning fires but the code is what the developer intended (ie: the existing code with the existing language semantics produce the desired result, the "fix" is to add parentheses that explicitly encode the language's existing rules/behavior anyway).

I agree with that definition -- that's a useful way to approach this, thank you!

Not that we don't have warnings that do this - that encourage parens to reinforce what the language already does to be more explicit/intentional about it, and in some cases it's not that uncommon that the user will be adding parens that reinforce the precedence rules anyway.

Yup, which is largely what this patch is about.

Like, I think all the fixes in libc++, llvm, etc, are false positives? (none of them found bugs/unintended behavior)

Yes, they all are false positives by the above definition.

Are there any examples of bugs being found by this warning in a codebase? (& how many false positives in such a codebase did it also flag?)

This would be good to know, but a bit of a heavy lift to require of @chaitanyav because they were working on this issue (https://github.com/llvm/llvm-project/issues/61943) with a "good first issue" label that is starting to look a bit like it was misleading (sorry about that!). However, if you're able to try compiling some larger projects with your patch applied to see if it spots any bugs in real world code, that would be very helpful!

@aaron.ballman will try this patch on some projects and post the results here

so we should pay close attention to user feedback during the 17.x release cycle. If it seems we're getting significant push back, we may need to come back and rethink.

Specifically, I think we've improved the situation for code like this:

// `p` is a pointer, `x` is an `int`, and `b` is a `bool`
p + x ? 1 : 2; // previously didn't warn, now warns
p + b ? 1 : 2; // always warned

Does anyone feel we should not move forward with accepting the patch in its current form?

*goes to look*

Mixed feelings - you're right, incremental improvement/adding missed cases to an existing warning (especially if that warning's already stylistic in nature) is a lower bar/doesn't necessarily need the false positive assessment. But it looks like this case might've been intentionally suppressed in the initial warning implementation? (anyone linked to the original warning implementation/review/design to check if this was intentional?)

I tried to chase down where this got added to see what review comments there were, but it seems to predate my involvement (I'm seeing mentions of this in 2011).

But, yeah, seems marginal enough I don't have strong feelings either way.

Thank you for the opinion! I think pointer + int is a far more common code pattern than pointer + bool, so it makes some sense to me that we would silence the first case while diagnosing the second case. Given the general lack of enthusiasm for the new diagnostics, it may boil down to answering whether this finds any true positives or not.

chaitanyav added a comment.EditedMay 23 2023, 6:28 PM

I used the patch to compile LLVM, apache/arrow, apache/trafficserver, folly, tensorstore, protobuf. I did not see any cases with pointer arithmetic in these repos. I have uploaded the make logs for the projects with diagnostic warnings.

Some examples

/home/nvellanki/scratch/temp_llvm/llvm-project/llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp:3721:49: warning: operator '?:' has lower precedence than '%'; '%' will be evaluated first [-Wparentheses]
  unsigned NumLeftovers = OrigNumElts % NumElts ? 1 : 0;
                          ~~~~~~~~~~~~~~~~~~~~~ ^
/home/nvellanki/scratch/temp_llvm/llvm-project/llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp:3721:49: note: place parentheses around the '%' expression to silence this warning
  unsigned NumLeftovers = OrigNumElts % NumElts ? 1 : 0;
                                                ^
                          (                    )
/home/nvellanki/scratch/temp_llvm/llvm-project/llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp:3721:49: note: place parentheses around the '?:' expression to evaluate it first
  unsigned NumLeftovers = OrigNumElts % NumElts ? 1 : 0;
                                                ^
                                        (              )
/home/nvellanki/scratch/temp_llvm/llvm-project/llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp:3775:49: warning: operator '?:' has lower precedence than '%'; '%' will be evaluated first [-Wparentheses]
  unsigned NumLeftovers = OrigNumElts % NumElts ? 1 : 0;
                          ~~~~~~~~~~~~~~~~~~~~~ ^
/home/nvellanki/scratch/tensorstore/build/_deps/zlib-src/crc32.c:572:19: warning: operator '?:' has lower precedence than '&'; '&' will be evaluated first [-Wbitwise-conditional-parentheses]
        b = b & 1 ? (b >> 1) ^ POLY : b >> 1;
            ~~~~~ ^
/home/nvellanki/scratch/tensorstore/build/_deps/zlib-src/crc32.c:572:19: note: place parentheses around the '&' expression to silence this warning
        b = b & 1 ? (b >> 1) ^ POLY : b >> 1;
                  ^
            (    )
/home/nvellanki/scratch/tensorstore/build/_deps/zlib-src/crc32.c:572:19: note: place parentheses around the '?:' expression to evaluate it first
        b = b & 1 ? (b >> 1) ^ POLY : b >> 1;
                  ^
                (                           )
/home/nvellanki/scratch/arrow/cpp/src/arrow/vendored/datetime/date.h:3941:61: warning: operator '?:' has lower precedence than '%'; '%' will be evaluated first [-Wparentheses]
    static CONSTDATA std::uint64_t value = h * h * (exp % 2 ? 10 : 1);
                                                    ~~~~~~~ ^
/home/nvellanki/scratch/arrow/cpp/src/arrow/vendored/datetime/date.h:3941:61: note: place parentheses around the '%' expression to silence this warning
    static CONSTDATA std::uint64_t value = h * h * (exp % 2 ? 10 : 1);
                                                            ^
                                                    (      )
/home/nvellanki/scratch/arrow/cpp/src/arrow/vendored/datetime/date.h:3941:61: note: place parentheses around the '?:' expression to evaluate it first
    static CONSTDATA std::uint64_t value = h * h * (exp % 2 ? 10 : 1);
                                                            ^
                                                          (         )
/home/nvellanki/scratch/trafficserver/include/tscore/SimpleTokenizer.h:145:47: warning: operator '?:' has lower precedence than '&'; '&' will be evaluated first [-Wbitwise-conditional-parentheses]
    _data   = (_mode & OVERWRITE_INPUT_STRING ? const_cast<char *>(s) : ats_strdup(s));
               ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^
/home/nvellanki/scratch/trafficserver/include/tscore/SimpleTokenizer.h:145:47: note: place parentheses around the '&' expression to silence this warning
    _data   = (_mode & OVERWRITE_INPUT_STRING ? const_cast<char *>(s) : ats_strdup(s));
                                              ^
               (                             )
/home/nvellanki/scratch/trafficserver/include/tscore/SimpleTokenizer.h:145:47: note: place parentheses around the '?:' expression to evaluate it first
    _data   = (_mode & OVERWRITE_INPUT_STRING ? const_cast<char *>(s) : ats_strdup(s));
                       ~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/nvellanki/scratch/protobuf/src/google/protobuf/map.h:773:47: warning: operator '?:' has lower precedence than '&'; '&' will be evaluated first [-Wbitwise-conditional-parentheses]
                      bits & kUseDestructFunc ? DestroyNode<Node> : nullptr};
                      ~~~~~~~~~~~~~~~~~~~~~~~ ^
/home/nvellanki/scratch/protobuf/src/google/protobuf/map.h:773:47: note: place parentheses around the '&' expression to silence this warning
                      bits & kUseDestructFunc ? DestroyNode<Node> : nullptr};
                                              ^
                      (                      )
/home/nvellanki/scratch/protobuf/src/google/protobuf/map.h:773:47: note: place parentheses around the '?:' expression to evaluate it first
                      bits & kUseDestructFunc ? DestroyNode<Node> : nullptr};
                                              ^
In file included from /home/nvellanki/scratch/folly/folly/lang/Bits.h:63:
/home/nvellanki/scratch/folly/folly/ConstexprMath.h:139:20: warning: operator '?:' has lower precedence than '%'; '%' will be evaluated first [-Wparentheses]
          (exp % 2 ? base : T(1));
           ~~~~~~~ ^
/home/nvellanki/scratch/folly/folly/ConstexprMath.h:139:20: note: place parentheses around the '%' expression to silence this warning
          (exp % 2 ? base : T(1));
                   ^
           (      )
/home/nvellanki/scratch/folly/folly/ConstexprMath.h:139:20: note: place parentheses around the '?:' expression to evaluate it first
          (exp % 2 ? base : T(1));
                   ^
/home/nvellanki/scratch/temp_llvm/llvm-project/llvm/lib/DebugInfo/DWARF/DWARFDebugMacro.cpp:24:36: warning: operator '?:' has lower precedence than '&'; '&' will be evaluated first [-Wbitwise-conditional-parentheses]
  return Flags & MACRO_OFFSET_SIZE ? DWARF64 : DWARF32;
         ~~~~~~~~~~~~~~~~~~~~~~~~~ ^
/home/nvellanki/scratch/temp_llvm/llvm-project/llvm/lib/DebugInfo/DWARF/DWARFDebugMacro.cpp:24:36: note: place parentheses around the '&' expression to silence this warning
  return Flags & MACRO_OFFSET_SIZE ? DWARF64 : DWARF32;
                                   ^
         (                        )
/home/nvellanki/scratch/temp_llvm/llvm-project/llvm/lib/DebugInfo/DWARF/DWARFDebugMacro.cpp:24:36: note: place parentheses around the '?:' expression to evaluate it first
  return Flags & MACRO_OFFSET_SIZE ? DWARF64 : DWARF32;
                                   ^
                 (                                    )
/home/nvellanki/scratch/temp_llvm/llvm-project/llvm/lib/CodeGen/MIRParser/MIParser.cpp:3381:49: warning: operator '?:' has lower precedence than '&'; '&' will be evaluated first [-Wbitwise-conditional-parentheses]
            : Flags & MachineMemOperand::MOLoad ? "from" : "into";
              ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^
/home/nvellanki/scratch/temp_llvm/llvm-project/llvm/lib/CodeGen/MIRParser/MIParser.cpp:3381:49: note: place parentheses around the '&' expression to silence this warning
            : Flags & MachineMemOperand::MOLoad ? "from" : "into";
                                                ^
              (                                )
/home/nvellanki/scratch/temp_llvm/llvm-project/llvm/lib/CodeGen/MIRParser/MIParser.cpp:3381:49: note: place parentheses around the '?:' expression to evaluate it first
            : Flags & MachineMemOperand::MOLoad ? "from" : "into";
                                                ^
                            ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^
/home/nvellanki/scratch/temp_llvm/llvm-project/llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:2377:5: note: place parentheses around the '&' expression to silence this warning
    GENBOOLCOMMENT(", ", VRData, HasVMXInstruction);
    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/nvellanki/scratch/temp_llvm/llvm-project/llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:2138:65: note: expanded from macro 'GENBOOLCOMMENT'
  CommentOS << (Prefix) << ((V) & (TracebackTable::Field##Mask) ? "+" : "-")   \
                            ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^
/home/nvellanki/scratch/temp_llvm/llvm-project/llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:2377:5: note: place parentheses around the '?:' expression to evaluate it first
    GENBOOLCOMMENT(", ", VRData, HasVMXInstruction);
    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/nvellanki/scratch/temp_llvm/llvm-project/llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:2138:65: note: expanded from macro 'GENBOOLCOMMENT'
  CommentOS << (Prefix) << ((V) & (TracebackTable::Field##Mask) ? "+" : "-")   \
                                  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~

yeah, doesn't look like it found any bugs there (the arrow one seems the most non-trivwial, but I'm guessing the code as-is is correct), so seems a bit questionable as an addition

But @aaron.ballman if you figure this falls more under the "minor tweak to existing diagnostic" I won't stand in the way of it.

Ping. What's missing to land this?

aaron.ballman requested changes to this revision.Sep 15 2023, 4:33 AM

Ping. What's missing to land this?

Folks have raised enough concerns about chattiness on the thread that I think we should not move forward without more evidence that the changes will find new true positives. I'm marking this as requesting changes so the review gets out of people's Phab queues, but the "changes" are largely about what evidence we find.

This revision now requires changes to proceed.Sep 15 2023, 4:33 AM

FWIW: adding parentheses for expressions, such as overflow = (4608 * 1024 * 1024) ? 4608 * 1024 * 1024 : 0;, and results.reason = (actions & _UA_SEARCH_PHASE) ? ... : ..., looks unnatural and is too noisy to me.

FWIW: adding parentheses for expressions, such as overflow = (4608 * 1024 * 1024) ? 4608 * 1024 * 1024 : 0;, and results.reason = (actions & _UA_SEARCH_PHASE) ? ... : ..., looks unnatural and is too noisy to me.

From quuxplusone:

The first expression

overflow = (4608 * 1024 * 1024) ?  4608 * 1024 * 1024 : 0;

is a tautology; it is not possible for (4608 * 1024 * 1024) to be zero. (If it's signed integer overflow, it's UB, not zero; and the compiler knows this.)
But if you make it defined by adding u, then IMO it's very little hardship to explicitly write the comparison to zero:

overflow = (4608u * 1024 * 1024) != 0 ?  4608u * 1024 * 1024 : 0;  // OK, no UB, no warning

The second expression

results.reason = (actions & _UA_SEARCH_PHASE) ? ... : ...

should IMHO also be written as

results.reason = (actions & _UA_SEARCH_PHASE) != 0 ? ... : ...

but I think you would get good support if you proposed that the compiler should treat & as a "logical operator" in this specific case. This just means changing line 9312 to

return OP->isComparisonOp() || OP->isLogicalOp() || (OP->getOpcode() == BO_And);

Rebase with upstream and update code as per comments

aaron.ballman added inline comments.Sep 26 2023, 10:00 AM
clang/docs/ReleaseNotes.rst
268–270

It looks like your rebase may have pulled in unintended changes.

Rebase with upstream

Bootstrapping build failed due to -werror flag (https://buildkite.com/llvm-project/libcxx-ci/builds/30031)

 | /home/libcxx-builder/.buildkite-agent/builds/google-libcxx-builder-4613821dc47f-1/llvm-project/libcxx-ci/libcxx/test/std/containers/unord/unord.multiset/eq.different_hash.pass.cpp:43:18: error: operator '?:' has lower precedence than '&'; '&' will be evaluated first [-Werror,-Wbitwise-conditional-parentheses]
# |    43 |   return val & 1 ? 1 : 0;
# |       |          ~~~~~~~ ^


# | /home/libcxx-builder/.buildkite-agent/builds/google-libcxx-builder-4613821dc47f-1/llvm-project/libcxx-ci/libcxx/test/std/containers/unord/unord.multiset/eq.different_hash.pass.cpp:64:19: error: operator '?:' has lower precedence than '&'; '&' will be evaluated first [-Werror,-Wbitwise-conditional-parentheses]
# |    64 |   return *val & 1 ? 1 : 0;
# |       |          ~~~~~~~~ ^

Bootstrapping build failed due to -werror flag (https://buildkite.com/llvm-project/libcxx-ci/builds/30031)

 | /home/libcxx-builder/.buildkite-agent/builds/google-libcxx-builder-4613821dc47f-1/llvm-project/libcxx-ci/libcxx/test/std/containers/unord/unord.multiset/eq.different_hash.pass.cpp:43:18: error: operator '?:' has lower precedence than '&'; '&' will be evaluated first [-Werror,-Wbitwise-conditional-parentheses]
# |    43 |   return val & 1 ? 1 : 0;
# |       |          ~~~~~~~ ^


# | /home/libcxx-builder/.buildkite-agent/builds/google-libcxx-builder-4613821dc47f-1/llvm-project/libcxx-ci/libcxx/test/std/containers/unord/unord.multiset/eq.different_hash.pass.cpp:64:19: error: operator '?:' has lower precedence than '&'; '&' will be evaluated first [-Werror,-Wbitwise-conditional-parentheses]
# |    64 |   return *val & 1 ? 1 : 0;
# |       |          ~~~~~~~~ ^

That looks like it is just in a test in libcxx, so at least that is a fairly simple thing to fix. I'd suggest just adding the parens in there (and I'm sure that is something that @ldionne would have no problem with), or adding the -Wno flag to the tests.

Add parentheses to the conditional expression

Add parentheses to the conditional expression

rebase upstream changes

chaitanyav marked an inline comment as done.Sep 30 2023, 5:25 PM

Ran llvm with the boolean and operator change. attaching the log file with errors .

Looks like there is quite a few places in the LLVM build that need updating (IteratorTest.cpp, Wasm.h, ElfDumper.cpp) so that builds don't fail as we commit this.

Rebase with upstream

rsmith added a subscriber: rsmith.Oct 4 2023, 10:43 AM

I've been pondering what I'd want from a warning here. I think generally I would like to warn if there are two plausible interpretations of the token sequence -- that is, if giving the ? different precedence could plausibly lead to a different valid program. I think concretely that would lead to this rule:

Warn if: the condition in a conditional-expression has a suffix (right-hand operand, recursively, looking only through binary operators) that is plausibly a condition. That is:

  • It is of a bool-like type: either bool itself, or an arithmetic or pointer or member pointer type, or a class with an operator bool.
  • It is not a constant expression.

Compared to this patch, I think the main change would be the second bullet: do *not* warn if the potential alternative condition is a constant expression -- that's not a plausible condition for a conditional expression. Looking through the test changes in this patch, this change would remove the vast majority of the false positives where it's obvious to me as a reader of the code that the code was already correct, and just leave a few changes like the second one in cxa_personality.cpp where the code really does look ambiguous as written.