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

[chaitanyav]

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.

[chaitanyav]

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]

@aaron.ballman Please review this.

[chaitanyav]

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

[aaron.ballman]

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?

[chaitanyav]

Update release notes about the fix

[aaron.ballman]

clang/test/Sema/parentheses.cpp

34

I'm still wondering about this.

[chaitanyav]

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

In D147844#4278384, @chaitanyav wrote:

@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).

[chaitanyav]

In D147844#4281288, @aaron.ballman wrote:

In D147844#4278384, @chaitanyav wrote:

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

[chaitanyav]

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

[chaitanyav]

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

[chaitanyav]

Update SemaCXX tests to use parentheses around conditional operator

[chaitanyav]

Update test to use parentheses around conditional operator

[chaitanyav]

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.

[chaitanyav]

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

[chaitanyav]

clang-format the changes to the file

[chaitanyav]

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

[chaitanyav]

@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

[philnik]

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.

In D147844#4286364, @chaitanyav wrote:

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

[chaitanyav]

Fix indentation

[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]

Modify the commit message

[chaitanyav]

Revise commit message

[Mordante]

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.

[aaron.ballman]

In D147844#4286500, @philnik wrote:

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]

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.

[dblaikie]

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?)

[cor3ntin]

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]

In D147844#4293693, @dblaikie wrote:

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.

[dblaikie]

In D147844#4293743, @cjdb wrote:

In D147844#4293693, @dblaikie wrote:

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.

[chaitanyav]

In D147844#4293299, @cjdb wrote:

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.

[chaitanyav]

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

[chaitanyav]

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

[chaitanyav]

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

[aaron.ballman]

In D147844#4293743, @cjdb wrote:

In D147844#4293693, @dblaikie wrote:

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]

In D147844#4293988, @dblaikie wrote:

In D147844#4293743, @cjdb wrote:

In D147844#4293693, @dblaikie wrote:

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 :)

[chaitanyav]

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

In D147844#4299418, @chaitanyav wrote:

@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

[chaitanyav]

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

[aaron.ballman]

In D147844#4299856, @chaitanyav wrote:

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.

[chaitanyav]

In D147844#4299874, @aaron.ballman wrote:

In D147844#4299856, @chaitanyav wrote:

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.

[chaitanyav]

Revert to earliest patch

[chaitanyav]

Add parentheses around integer expressions in libcxx tests

[chaitanyav]

Revert libcxx libcxxabi files unrelated to the issue

[chaitanyav]

Rebase with upstream

[chaitanyav]

Rebase with upstream

[chaitanyav]

Rebase with upstream

[aaron.ballman]

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]

Rebase with upstream and remove duplicate line from ReleaseNotes

[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.

[chaitanyav]

Remove extra parens

[chaitanyav]

Rebase with upstream

[aaron.ballman]

In D147844#4333407, @ldionne wrote:

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).

[dblaikie]

In D147844#4329497, @aaron.ballman wrote:

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.

[chaitanyav]

Rebase with upstream

[chaitanyav]

Rebase with upstream

[chaitanyav]

Rebase with upstream

[aaron.ballman]

In D147844#4335598, @dblaikie wrote:

In D147844#4329497, @aaron.ballman wrote:

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.

[chaitanyav]

In D147844#4361956, @aaron.ballman wrote:

In D147844#4335598, @dblaikie wrote:

In D147844#4329497, @aaron.ballman wrote:

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]

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.

llvm_out.txt1 MBDownload

apache_arrow_cpp_out.txt155 KBDownload

apache_trafficserver_out.txt1011 KBDownload

protobuf_out.txt1 MBDownload

folly_out.txt484 KBDownload

tensorstore_out.txt597 KBDownload

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) ? "+" : "-")   \
                                  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~

[dblaikie]

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.

[ldionne]

Ping. What's missing to land this?

[aaron.ballman]

In D147844#4641735, @ldionne wrote:

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.

[MaskRay]

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.

[MaskRay]

In D147844#4646633, @MaskRay wrote:

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);

[chaitanyav]

Rebase with upstream and update code as per comments

[aaron.ballman]

clang/docs/ReleaseNotes.rst

268–270

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

[chaitanyav]

Rebase with upstream

[chaitanyav]

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;
# |       |          ~~~~~~~~ ^

[erichkeane]

In D147844#4651113, @chaitanyav wrote:

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.

[chaitanyav]

Add parentheses to the conditional expression

[chaitanyav]

Add parentheses to the conditional expression

[chaitanyav]

rebase upstream changes

[chaitanyav]

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

llvm.txt1 MBDownload

[erichkeane]

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.

[chaitanyav]

Rebase with upstream

[rsmith]

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.

clang/docs/ReleaseNotes.rst

- ``-Wformat`` now recognizes ``%lb`` for the ``printf``/``scanf`` family of
   functions.
   (`#62247: <https://github.com/llvm/llvm-project/issues/62247>`_).
 - Clang now diagnoses shadowing of lambda's template parameter by a capture.
   (`#61105: <https://github.com/llvm/llvm-project/issues/61105>`_).
 
 Bug Fixes in This Version
 -------------------------
 
 - Fix segfault while running clang-rename on a non existing file.
   (`#36471 <https://github.com/llvm/llvm-project/issues/36471>`_)
 - Fix crash when diagnosing incorrect usage of ``_Nullable`` involving alias
   templates.
   (`#60344 <https://github.com/llvm/llvm-project/issues/60344>`_)
 - Fix confusing warning message when ``/clang:-x`` is passed in ``clang-cl``
   driver mode and emit an error which suggests using ``/TC`` or ``/TP``
   ``clang-cl`` options instead.
   (`#59307 <https://github.com/llvm/llvm-project/issues/59307>`_)
 - Fix assert that fails when the expression causing the this pointer to be
   captured by a block is part of a constexpr if statement's branch and

 - Fix the assertion hit when generating code for global variable initializer of
   _BitInt(1) type.
   (`#62207 <https://github.com/llvm/llvm-project/issues/62207>`_)
 - Fix lambdas and other anonymous function names not respecting ``-fdebug-prefix-map``
   (`#62192 <https://github.com/llvm/llvm-project/issues/62192>`_)
 - Fix crash when attempting to pass a non-pointer type as first argument of
   ``__builtin_assume_aligned``.
   (`#62305 <https://github.com/llvm/llvm-project/issues/62305>`_)
+  (`#62305 <https://github.com/llvm/llvm-project/issues/62305>`_)
+- Print diagnostic warning about precedence when integer expression is used
+  without parentheses in an conditional operator expression
+  (`#61943 <https://github.com/llvm/llvm-project/issues/61943>`_)
 
 Bug Fixes to Compiler Builtins
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
 Bug Fixes to Attribute Support
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 - Fixed a bug where attribute annotations on type specifiers (enums, classes,
   structs, unions, and scoped enums) were not properly ignored, resulting in

clang/lib/Sema/SemaExpr.cpp

static bool ExprLooksBoolean(Expr *E) {
   if (E->getType()->isBooleanType())
     return true;
   if (BinaryOperator *OP = dyn_cast<BinaryOperator>(E))
     return OP->isComparisonOp() || OP->isLogicalOp();
   if (UnaryOperator *OP = dyn_cast<UnaryOperator>(E))
     return OP->getOpcode() == UO_LNot;
   if (E->getType()->isPointerType())
     return true;
+  if (E->getType()->isIntegerType())
+    return true;
   // FIXME: What about overloaded operator calls returning "unspecified boolean
   // type"s (commonly pointer-to-members)?
 
   return false;
 }
 
 /// DiagnoseConditionalPrecedence - Emit a warning when a conditional operator
 /// and binary operator are mixed in a way that suggests the programmer assumed

clang/test/Analysis/uninit-vals.c

if (x+1)  // expected-warning{{The left operand of '+' is a garbage value}}
     return 1;
     
   return 2;  
 }
 
 int f2_b(void) {
   int x; // expected-note{{'x' declared without an initial value}}
   
-  return ((1+x)+2+((x))) + 1 ? 1 : 2; // expected-warning{{The right operand of '+' is a garbage value}}
+  return (((1+x)+2+((x))) + 1) ? 1 : 2; // expected-warning{{The right operand of '+' is a garbage value}}
                                       // expected-note@-1{{The right operand of '+' is a garbage value}}
 }
 
 int f3(void) {
   int i; // expected-note{{'i' declared without an initial value}}
   int *p = &i;
   if (*p > 0) // expected-warning{{The left operand of '>' is a garbage value}}
               // expected-note@-1{{The left operand of '>' is a garbage value}}

clang/test/Sema/integer-overflow.c

// expected-warning@+1 {{overflow in expression; result is 536870912 with type 'int'}}
   overflow += overflow2 = overflow3 = (uint64_t)(4608 * 1024 * 1024);
 // expected-warning@+1 {{overflow in expression; result is 536870912 with type 'int'}}
   overflow += overflow2 = overflow3 = 4608 * 1024 * 1024;
 
   uint64_t not_overflow = 4608 * 1024 * 1024ULL;
   uint64_t not_overflow2 = (1ULL * ((uint64_t)(4608) * (1024 * 1024)) + 2ULL);
 
 // expected-warning@+1 2{{overflow in expression; result is 536870912 with type 'int'}}
-  overflow = 4608 * 1024 * 1024 ?  4608 * 1024 * 1024 : 0;
+  overflow = (4608 * 1024 * 1024) ?  4608 * 1024 * 1024 : 0;
 
 // expected-warning@+1 {{overflow in expression; result is 536870912 with type 'int'}}
   overflow =  0 ? 0 : 4608 * 1024 * 1024;
 
 // expected-warning@+1 {{overflow in expression; result is 536870912 with type 'int'}}
   if (4608 * 1024 * 1024)
     return 0;
 

clang/test/Sema/parentheses.c

-// RUN: %clang_cc1 -fsyntax-only -verify %s
+// RUN: %clang_cc1 -Wparentheses -fsyntax-only -verify %s
 // RUN: %clang_cc1 -Wparentheses -fsyntax-only -verify %s
 // RUN: %clang_cc1 -Wparentheses -fsyntax-only -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
 
 // Test the various warnings under -Wparentheses
 void if_assign(void) {
   int i;
   if (i = 4) {} // expected-warning {{assignment as a condition}} \
                 // expected-note{{place parentheses around the assignment to silence this warning}} \
void conditional_op(int x, int y, _Bool b, void* p) {
   (void)(x / !x ? 1 : 2); // expected-warning {{operator '?:' has lower precedence than '/'}} \
                           // expected-note {{place parentheses around the '/' expression to silence this warning}} \
                           // expected-note {{place parentheses around the '?:' expression to evaluate it first}}
   // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:10-[[@LINE-3]]:10}:"("
   // CHECK: fix-it:"{{.*}}":{[[@LINE-4]]:16-[[@LINE-4]]:16}:")"
   // CHECK: fix-it:"{{.*}}":{[[@LINE-5]]:14-[[@LINE-5]]:14}:"("
   // CHECK: fix-it:"{{.*}}":{[[@LINE-6]]:24-[[@LINE-6]]:24}:")"
 
-  (void)(x % 2 ? 1 : 2); // no warning
+  (void)(x % 2 ? 1 : 2); // expected-warning {{operator '?:' has lower precedence than '%'}} expected-note 2{{place parenthese}}
 
   (void)(x + p ? 1 : 2); // expected-warning {{operator '?:' has lower precedence than '+'}} expected-note 2{{place parentheses}}
-  (void)(p + x ? 1 : 2); // no warning
+  (void)(p + x ? 1 : 2); // expected-warning {{operator '?:' has lower precedence than '+'}} expected-note 2{{place parentheses}}
 
   (void)(p + b ? 1 : 2); // expected-warning {{operator '?:' has lower precedence than '+'}} expected-note 2{{place parentheses}}
 
   (void)(x + y > 0 ? 1 : 2); // no warning
   (void)(x + (y > 0) ? 1 : 2); // expected-warning {{operator '?:' has lower precedence than '+'}} expected-note 2{{place parentheses}}
 
   (void)(b ? 0xf0 : 0x10 | b ? 0x5 : 0x2); // expected-warning {{operator '?:' has lower precedence than '|'}} expected-note 2{{place parentheses}}
 
   (void)((b ? 0xf0 : 0x10) | (b ? 0x5 : 0x2));  // no warning, has parentheses
   (void)(b ? 0xf0 : (0x10 | b) ? 0x5 : 0x2);  // no warning, has parentheses
 
   (void)(x | b ? 1 : 2); // expected-warning {{operator '?:' has lower precedence than '|'}} expected-note 2{{place parentheses}}

clang/test/Sema/parentheses.cpp

-// RUN: %clang_cc1 -Wparentheses -fsyntax-only -verify %s
-// RUN: %clang_cc1 -Wparentheses -fsyntax-only -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
+// RUN: %clang_cc1 -Wparentheses -Woverloaded-shift-op-parentheses -fsyntax-only -verify %s
+// RUN: %clang_cc1 -Wparentheses -Woverloaded-shift-op-parentheses -fsyntax-only -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
 
 bool someConditionFunc();
 
 void conditional_op(int x, int y, bool b) {
   (void)(x + someConditionFunc() ? 1 : 2); // expected-warning {{operator '?:' has lower precedence than '+'}} \
                                            // expected-note {{place parentheses around the '+' expression to silence this warning}} \
                                            // expected-note {{place parentheses around the '?:' expression to evaluate it first}}
   // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:10-[[@LINE-3]]:10}:"("
void conditional_op(int x, int y, bool b) {
   // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:10-[[@LINE-3]]:10}:"("
   // CHECK: fix-it:"{{.*}}":{[[@LINE-4]]:22-[[@LINE-4]]:22}:")"
   // CHECK: fix-it:"{{.*}}":{[[@LINE-5]]:14-[[@LINE-5]]:14}:"("
   // CHECK: fix-it:"{{.*}}":{[[@LINE-6]]:30-[[@LINE-6]]:30}:")"
 }
 
 class Stream {
 public:
   operator int();
   Stream &operator<<(int);
   Stream &operator<<(const char*);
   Stream &operator>>(int);
   Stream &operator>>(const char*);
 };
 
-void f(Stream& s, bool b) {
+void f(Stream& s, bool b, int x) {
   (void)(s << b ? "foo" : "bar"); // expected-warning {{operator '?:' has lower precedence than '<<'}} \
                                   // expected-note {{place parentheses around the '<<' expression to silence this warning}} \
                                   // expected-note {{place parentheses around the '?:' expression to evaluate it first}}
   // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:10-[[@LINE-3]]:10}:"("
   // CHECK: fix-it:"{{.*}}":{[[@LINE-4]]:16-[[@LINE-4]]:16}:")"
   // CHECK: fix-it:"{{.*}}":{[[@LINE-5]]:15-[[@LINE-5]]:15}:"("
   // CHECK: fix-it:"{{.*}}":{[[@LINE-6]]:32-[[@LINE-6]]:32}:")"
 
   (void)(s << 5 == 1); // expected-warning {{overloaded operator << has higher precedence than comparison operator}} \
                        // expected-note {{place parentheses around the '<<' expression to silence this warning}} \
                        // expected-note {{place parentheses around comparison expression to evaluate it first}}
   // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:10-[[@LINE-3]]:10}:"("
   // CHECK: fix-it:"{{.*}}":{[[@LINE-4]]:16-[[@LINE-4]]:16}:")"
   // CHECK: fix-it:"{{.*}}":{[[@LINE-5]]:15-[[@LINE-5]]:15}:"("
   // CHECK: fix-it:"{{.*}}":{[[@LINE-6]]:21-[[@LINE-6]]:21}:")"
 
   (void)(s >> 5 == 1); // expected-warning {{overloaded operator >> has higher precedence than comparison operator}} \
                        // expected-note {{place parentheses around the '>>' expression to silence this warning}} \
                        // expected-note {{place parentheses around comparison expression to evaluate it first}}
   // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:10-[[@LINE-3]]:10}:"("
   // CHECK: fix-it:"{{.*}}":{[[@LINE-4]]:16-[[@LINE-4]]:16}:")"
   // CHECK: fix-it:"{{.*}}":{[[@LINE-5]]:15-[[@LINE-5]]:15}:"("
   // CHECK: fix-it:"{{.*}}":{[[@LINE-6]]:21-[[@LINE-6]]:21}:")"
+
+  void(s << "Test" << x ? "foo" : "bar"); //expected-warning {{operator '?:' has lower precedence than '<<'}} \
+                                         // expected-note {{place parentheses around the '<<' expression to silence this warning}} \
+                                        // expected-note {{place parentheses around the '?:' expression to evaluate it first}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:8-[[@LINE-3]]:8}:"("
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-4]]:24-[[@LINE-4]]:24}:")"
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-5]]:23-[[@LINE-5]]:23}:"("
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-6]]:40-[[@LINE-6]]:40}:")"
+
+  void(s << x == 1 ? "foo": "bar"); //expected-warning {{overloaded operator << has higher precedence than comparison operator}} \
+                                    //expected-note {{place parentheses around the '<<' expression to silence this warning}} \
+                                    //expected-note {{place parentheses around comparison expression to evaluate it first}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:8-[[@LINE-3]]:8}:"("
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-4]]:14-[[@LINE-4]]:14}:")"
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-5]]:13-[[@LINE-5]]:13}:"("
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-6]]:19-[[@LINE-6]]:19}:")"
+
+  void(s << static_cast<bool>(x) ? "foo" : "bar"); //expected-warning {{operator '?:' has lower precedence than '<<'}} \
+                                                  //expected-note {{place parentheses around the '<<' expression to silence this warning}} \
+                                                 //expected-note {{place parentheses around the '?:' expression to evaluate it first}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:8-[[@LINE-3]]:8}:"("
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-4]]:33-[[@LINE-4]]:33}:")"
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-5]]:13-[[@LINE-5]]:13}:"("
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-6]]:49-[[@LINE-6]]:49}:")"
 }
 
 struct S {
   operator int() { return 42; }
   friend S operator+(const S &lhs, bool) { return S(); }
 };
 
 void test(S *s, bool (S::*m_ptr)()) {

clang/test/SemaCXX/array-bounds.cpp

template <int I> void f() {
   s.arr[I] = 0; // expected-warning {{array index 5 is past the end of the array (that has type 'char[3]')}}
 }
 
 void test_templates() {
   f<5>(); // expected-note {{in instantiation}}
 }
 
 #define SIZE 10
-#define ARR_IN_MACRO(flag, arr, idx) flag ? arr[idx] : 1
+#define ARR_IN_MACRO(flag, arr, idx) (flag ? arr[idx] : 1)
 
 int test_no_warn_macro_unreachable() {
   int arr[SIZE]; // expected-note {{array 'arr' declared here}}
   return ARR_IN_MACRO(0, arr, SIZE) + // no-warning
          ARR_IN_MACRO(1, arr, SIZE); // expected-warning{{array index 10 is past the end of the array (that has type 'int[10]')}}
 }
 
 // This exhibited an assertion failure for a 32-bit build of Clang.

clang/test/SemaCXX/integer-overflow.cpp

 
 // expected-warning@+1 {{overflow in expression; result is 536870912 with type 'int'}}
   overflow += overflow2 = overflow3 = static_cast<uint64_t>(4608 * 1024 * 1024);
 
   uint64_t not_overflow = 4608 * 1024 * 1024ULL;
   uint64_t not_overflow2 = (1ULL * ((uint64_t)(4608) * (1024 * 1024)) + 2ULL);
 
 // expected-warning@+1 2{{overflow in expression; result is 536870912 with type 'int'}}
-  overflow = 4608 * 1024 * 1024 ?  4608 * 1024 * 1024 : 0;
+  overflow = (4608 * 1024 * 1024) ?  4608 * 1024 * 1024 : 0;
 
 // expected-warning@+1 {{overflow in expression; result is 536870912 with type 'int'}}
   overflow =  0 ? 0 : 4608 * 1024 * 1024;
 
 // expected-warning@+1 {{overflow in expression; result is 536870912 with type 'int'}}
   if (4608 * 1024 * 1024)
     return 0;
 

libcxx/include/__chrono/duration.h

round(const duration<_Rep, _Period>& __d)
     _ToDuration __lower = chrono::floor<_ToDuration>(__d);
     _ToDuration __upper = __lower + _ToDuration{1};
     auto __lower_diff   = __d - __lower;
     auto __upper_diff   = __upper - __d;
     if (__lower_diff < __upper_diff)
         return __lower;
     if (__lower_diff > __upper_diff)
         return __upper;
-    return __lower.count() & 1 ? __upper : __lower;
+    return (__lower.count() & 1) ? __upper : __lower;
 }
 #endif
 
 // duration
 
 template <class _Rep, class _Period>
 class _LIBCPP_TEMPLATE_VIS duration
 {

libcxx/include/strstream

 {
 public:
     _LIBCPP_INLINE_VISIBILITY
     ostrstream()
         : ostream(&__sb_) {}
     _LIBCPP_INLINE_VISIBILITY
     ostrstream(char* __s, int __n, ios_base::openmode __mode = ios_base::out)
         : ostream(&__sb_),
-          __sb_(__s, __n, __s + (__mode & ios::app ? _VSTD::strlen(__s) : 0))
+          __sb_(__s, __n, __s + ((__mode & ios::app) ? _VSTD::strlen(__s) : 0))
         {}
 
 #ifndef _LIBCPP_CXX03_LANG
     _LIBCPP_INLINE_VISIBILITY
     ostrstream(ostrstream&& __rhs)
         : ostream(_VSTD::move(__rhs)),
           __sb_(_VSTD::move(__rhs.__sb_))
     {
public:
 
     // constructors/destructor
     _LIBCPP_INLINE_VISIBILITY
     strstream()
         : iostream(&__sb_) {}
     _LIBCPP_INLINE_VISIBILITY
     strstream(char* __s, int __n, ios_base::openmode __mode = ios_base::in | ios_base::out)
         : iostream(&__sb_),
-          __sb_(__s, __n, __s + (__mode & ios::app ? _VSTD::strlen(__s) : 0))
+          __sb_(__s, __n, __s + ((__mode & ios::app) ? _VSTD::strlen(__s) : 0))
         {}
 
 #ifndef _LIBCPP_CXX03_LANG
     _LIBCPP_INLINE_VISIBILITY
     strstream(strstream&& __rhs)
         : iostream(_VSTD::move(__rhs)),
           __sb_(_VSTD::move(__rhs.__sb_))
     {

libcxx/test/libcxx/algorithms/nth_element_stability.pass.cpp

struct MyType {
   constexpr bool operator<(const MyType& other) const { return value < other.value; }
 };
 
 std::vector<MyType> deterministic() {
   static constexpr int kSize = 100;
   std::vector<MyType> v;
   v.resize(kSize);
   for (int i = 0; i < kSize; ++i) {
-    v[i].value = (i % 2 ? i : kSize / 2 + i);
+    v[i].value = ((i % 2) ? i : kSize / 2 + i);
   }
   std::__nth_element<std::_ClassicAlgPolicy>(v.begin(), v.begin() + kSize / 2, v.end(), std::less<MyType>());
   return v;
 }
 
 void test_randomization() {
   static constexpr int kSize = 100;
   std::vector<MyType> v;
   v.resize(kSize);
   for (int i = 0; i < kSize; ++i) {
-    v[i].value = (i % 2 ? i : kSize / 2 + i);
+    v[i].value = ((i % 2) ? i : kSize / 2 + i);
   }
   auto deterministic_v = deterministic();
   std::nth_element(v.begin(), v.begin() + kSize / 2, v.end());
   bool all_equal = true;
   for (int i = 0; i < kSize; ++i) {
     if (v[i].value != deterministic_v[i].value) {
       all_equal = false;
     }
   }
   assert(!all_equal);
 }
 
 void test_same() {
   static constexpr int kSize = 100;
   std::vector<MyType> v;
   v.resize(kSize);
   for (int i = 0; i < kSize; ++i) {
-    v[i].value = (i % 2 ? i : kSize / 2 + i);
+    v[i].value = ((i % 2) ? i : kSize / 2 + i);
   }
   auto snapshot_v = v;
   auto snapshot_custom_v = v;
   std::nth_element(v.begin(), v.begin() + kSize / 2, v.end());
   std::nth_element(snapshot_v.begin(), snapshot_v.begin() + kSize / 2, snapshot_v.end());
   std::nth_element(snapshot_custom_v.begin(), snapshot_custom_v.begin() + kSize / 2, snapshot_custom_v.end(), std::less<MyType>());
   bool all_equal = true;
   for (int i = 0; i < kSize; ++i) {

libcxx/test/libcxx/algorithms/partial_sort_stability.pass.cpp

struct MyType {
   constexpr bool operator<(const MyType& other) const { return value < other.value; }
 };
 
 std::vector<MyType> deterministic() {
   static constexpr int kSize = 100;
   std::vector<MyType> v;
   v.resize(kSize);
   for (int i = 0; i < kSize; ++i) {
-    v[i].value = (i % 2 ? 1 : kSize / 2 + i);
+    v[i].value = ((i % 2) ? 1 : kSize / 2 + i);
   }
   auto comp = std::less<MyType>();
   std::__partial_sort_impl<std::_ClassicAlgPolicy>(v.begin(), v.begin() + kSize / 2, v.end(), comp);
   return v;
 }
 
 void test_randomization() {
   static constexpr int kSize = 100;
   std::vector<MyType> v;
   v.resize(kSize);
   for (int i = 0; i < kSize; ++i) {
-    v[i].value = (i % 2 ? 1 : kSize / 2 + i);
+    v[i].value = ((i % 2) ? 1 : kSize / 2 + i);
   }
   auto deterministic_v = deterministic();
   std::partial_sort(v.begin(), v.begin() + kSize / 2, v.end());
   bool all_equal = true;
   for (int i = 0; i < kSize; ++i) {
     if (v[i].value != deterministic_v[i].value) {
       all_equal = false;
     }
   }
   assert(!all_equal);
 }
 
 void test_same() {
   static constexpr int kSize = 100;
   std::vector<MyType> v;
   v.resize(kSize);
   for (int i = 0; i < kSize; ++i) {
-    v[i].value = (i % 2 ? 1 : kSize / 2 + i);
+    v[i].value = ((i % 2) ? 1 : kSize / 2 + i);
   }
   auto snapshot_v = v;
   auto snapshot_custom_v = v;
   std::partial_sort(v.begin(), v.begin() + kSize / 2, v.end());
   std::partial_sort(snapshot_v.begin(), snapshot_v.begin() + kSize / 2, snapshot_v.end());
   std::partial_sort(snapshot_custom_v.begin(), snapshot_custom_v.begin() + kSize / 2, snapshot_custom_v.end(), std::less<MyType>());
   bool all_equal = true;
   for (int i = 0; i < kSize; ++i) {

libcxx/test/libcxx/algorithms/sort_stability.pass.cpp

struct EqualType {
   constexpr bool operator<(const EqualType&) const { return false; }
 };
 
 std::vector<EqualType> deterministic() {
   static constexpr int kSize = 100;
   std::vector<EqualType> v;
   v.resize(kSize);
   for (int i = 0; i < kSize; ++i) {
-    v[i].value = kSize / 2 - i * (i % 2 ? -1 : 1);
+    v[i].value = kSize / 2 - i * ((i % 2) ? -1 : 1);
   }
   std::less<EqualType> comp;
   std::__sort_dispatch<std::_ClassicAlgPolicy>(v.begin(), v.end(), comp);
   return v;
 }
 
 void test_randomization() {
   static constexpr int kSize = 100;
   std::vector<EqualType> v;
   v.resize(kSize);
   for (int i = 0; i < kSize; ++i) {
-    v[i].value = kSize / 2 - i * (i % 2 ? -1 : 1);
+    v[i].value = kSize / 2 - i * ((i % 2) ? -1 : 1);
   }
   auto deterministic_v = deterministic();
   std::sort(v.begin(), v.end());
   bool all_equal = true;
   for (int i = 0; i < kSize; ++i) {
     if (v[i].value != deterministic_v[i].value) {
       all_equal = false;
     }
   }
   assert(!all_equal);
 }
 
 void test_same() {
   static constexpr int kSize = 100;
   std::vector<EqualType> v;
   v.resize(kSize);
   for (int i = 0; i < kSize; ++i) {
-    v[i].value = kSize / 2 - i * (i % 2 ? -1 : 1);
+    v[i].value = kSize / 2 - i * ((i % 2) ? -1 : 1);
   }
   auto snapshot_v = v;
   auto snapshot_custom_v = v;
   std::sort(v.begin(), v.end());
   std::sort(snapshot_v.begin(), snapshot_v.end());
   std::sort(snapshot_custom_v.begin(), snapshot_custom_v.end(),
             [](const EqualType&, const EqualType&) { return false; });
   bool all_equal = true;

libcxxabi/src/cxa_personality.cpp

#ifndef __USING_SJLJ_EXCEPTIONS__
             landingPad = (uintptr_t)lpStart + landingPad;
 #else  // __USING_SJLJ_EXCEPTIONS__
             ++landingPad;
 #endif // __USING_SJLJ_EXCEPTIONS__
             results.landingPad = landingPad;
             if (actionEntry == 0)
             {
                 // Found a cleanup
-                results.reason = actions & _UA_SEARCH_PHASE
-                                     ? _URC_CONTINUE_UNWIND
-                                     : _URC_HANDLER_FOUND;
+                results.reason = ((actions & _UA_SEARCH_PHASE) ? _URC_CONTINUE_UNWIND : _URC_HANDLER_FOUND);
                 return;
             }
             // Convert 1-based byte offset into
             const uint8_t* action = actionTableStart + (actionEntry - 1);
             bool hasCleanup = false;
             // Scan action entries until you find a matching handler, cleanup, or the end of action list
             while (true)
             {
#endif // __USING_SJLJ_EXCEPTIONS__
                 }
                 const uint8_t* temp = action;
                 int64_t actionOffset = readSLEB128(&temp);
                 if (actionOffset == 0)
                 {
                     // End of action list. If this is phase 2 and we have found
                     // a cleanup (ttypeIndex=0), return _URC_HANDLER_FOUND;
                     // otherwise return _URC_CONTINUE_UNWIND.
-                    results.reason = hasCleanup && actions & _UA_CLEANUP_PHASE
-                                         ? _URC_HANDLER_FOUND
-                                         : _URC_CONTINUE_UNWIND;
+                    results.reason =
+                        (hasCleanup && (actions & _UA_CLEANUP_PHASE)) ? _URC_HANDLER_FOUND : _URC_CONTINUE_UNWIND;
                     return;
                 }
                 // Go to next action
                 action += actionOffset;
             }  // there is no break out of this loop, only return
         }
 #ifndef __USING_SJLJ_EXCEPTIONS__
         else if (ipOffset < start)
catch (...)
                 // This shouldn't be able to happen!
                 std::__terminate(t_handler);
             bool native_new_exception = __isOurExceptionClass(&new_exception_header->unwindHeader);
             void* adjustedPtr;
             if (native_new_exception && (new_exception_header != old_exception_header))
             {
                 const __shim_type_info* excpType =
                     static_cast<const __shim_type_info*>(new_exception_header->exceptionType);
-                adjustedPtr =
-                    __getExceptionClass(&new_exception_header->unwindHeader) == kOurDependentExceptionClass ?
-                        ((__cxa_dependent_exception*)new_exception_header)->primaryException :
-                        new_exception_header + 1;
+                adjustedPtr = (__getExceptionClass(&new_exception_header->unwindHeader) == kOurDependentExceptionClass)
+                                  ? ((__cxa_dependent_exception*)new_exception_header)->primaryException
+                                  : new_exception_header + 1;
                 if (!exception_spec_can_catch(ttypeIndex, classInfo, ttypeEncoding,
                                               excpType, adjustedPtr,
                                               unwind_exception, base))
                 {
                     // We need to __cxa_end_catch, but for the old exception,
                     //   not the new one.  This is a little tricky ...
                     // Disguise new_exception_header as a rethrown exception, but
                     //   don't actually rethrow it.  This means you can temporarily