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

rsmith added a comment. 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

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

chaitanyav updated this revision to Diff 557588. chaitanyav added a comment. Rebase with upstream Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D147844/new/ https://reviews.llvm.org/D147844 Files: clang/docs/ReleaseNotes.rst

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

erichkeane added a comment. 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. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

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

chaitanyav marked an inline comment as done. chaitanyav added a comment. Ran llvm with the boolean and operator change. attaching the log file with errors .F29524460: llvm.txt Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

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

chaitanyav updated this revision to Diff 557513. chaitanyav added a comment. rebase upstream changes Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D147844/new/ https://reviews.llvm.org/D147844 Files: clang/docs/ReleaseNotes.rst

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

chaitanyav updated this revision to Diff 557427. chaitanyav added a comment. Add parentheses to the conditional expression Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D147844/new/ https://reviews.llvm.org/D147844 Files:

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

chaitanyav updated this revision to Diff 557417. chaitanyav added a comment. Add parentheses to the conditional expression Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D147844/new/ https://reviews.llvm.org/D147844 Files:

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

erichkeane added a comment. In D147844#4651113 , @chaitanyav wrote: > Bootstrapping build failed due to -werror flag > (https://buildkite.com/llvm-project/libcxx-ci/builds/30031) > >| >

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

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

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

chaitanyav updated this revision to Diff 557373. chaitanyav added a comment. Rebase with upstream Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D147844/new/ https://reviews.llvm.org/D147844 Files: clang/docs/ReleaseNotes.rst

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

aaron.ballman added inline comments. Comment at: clang/docs/ReleaseNotes.rst:267-269 +- Fix segfault while running clang-rename on a non existing file. + (`#36471 `_) +- Fix crash when diagnosing incorrect usage of

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

chaitanyav updated this revision to Diff 557346. chaitanyav added a comment. Rebase with upstream and update code as per comments Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D147844/new/ https://reviews.llvm.org/D147844 Files:

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

MaskRay added a comment. 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

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

MaskRay added a comment. 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. Repository: rG LLVM Github Monorepo CHANGES

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

aaron.ballman requested changes to this revision. aaron.ballman added a comment. This revision now requires changes to proceed. In D147844#4641735 , @ldionne wrote: > Ping. What's missing to land this? Folks have raised enough concerns about chattiness

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

ldionne added a comment. Ping. What's missing to land this? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D147844/new/ https://reviews.llvm.org/D147844 ___ cfe-commits mailing list

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

dblaikie added a comment. 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

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

chaitanyav added a comment. 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 see there is some value for the patch in terms of readability (will be helpful to someone who is

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

chaitanyav added a comment. In D147844#4361956 , @aaron.ballman wrote: > In D147844#4335598 , @dblaikie > wrote: > >> In D147844#4329497 , >> @aaron.ballman wrote: >>

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

aaron.ballman added a comment. In D147844#4335598 , @dblaikie wrote: > In D147844#4329497 , @aaron.ballman > wrote: > >> In general, I think this is incremental progress on the diagnostic behavior. >> However,

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

chaitanyav updated this revision to Diff 524376. chaitanyav added a comment. Rebase with upstream Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D147844/new/ https://reviews.llvm.org/D147844 Files: clang/docs/ReleaseNotes.rst

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

chaitanyav updated this revision to Diff 524321. chaitanyav added a comment. Rebase with upstream Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D147844/new/ https://reviews.llvm.org/D147844 Files: clang/docs/ReleaseNotes.rst

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

chaitanyav updated this revision to Diff 524072. chaitanyav added a comment. Rebase with upstream Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D147844/new/ https://reviews.llvm.org/D147844 Files: clang/docs/ReleaseNotes.rst

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

dblaikie added a comment. 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

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

aaron.ballman added a comment. 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

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

chaitanyav updated this revision to Diff 521330. chaitanyav added a comment. Rebase with upstream Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D147844/new/ https://reviews.llvm.org/D147844 Files: clang/docs/ReleaseNotes.rst

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

chaitanyav updated this revision to Diff 521203. chaitanyav added a comment. Remove extra parens Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D147844/new/ https://reviews.llvm.org/D147844 Files: clang/docs/ReleaseNotes.rst

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

ldionne accepted this revision as: libc++abi. ldionne added a comment. This revision is now accepted and ready to land. 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.

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

chaitanyav updated this revision to Diff 520725. chaitanyav added a comment. Rebase with upstream and remove duplicate line from ReleaseNotes Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D147844/new/ https://reviews.llvm.org/D147844 Files:

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

aaron.ballman added a comment. 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

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

chaitanyav updated this revision to Diff 520491. chaitanyav added a comment. Rebase with upstream Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D147844/new/ https://reviews.llvm.org/D147844 Files: clang/docs/ReleaseNotes.rst

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

chaitanyav updated this revision to Diff 519259. chaitanyav added a comment. Rebase with upstream Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D147844/new/ https://reviews.llvm.org/D147844 Files: clang/docs/ReleaseNotes.rst

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

chaitanyav updated this revision to Diff 518118. chaitanyav added a comment. Rebase with upstream Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D147844/new/ https://reviews.llvm.org/D147844 Files: clang/docs/ReleaseNotes.rst

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

chaitanyav updated this revision to Diff 517429. chaitanyav added a comment. Revert libcxx libcxxabi files unrelated to the issue Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D147844/new/ https://reviews.llvm.org/D147844 Files:

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

chaitanyav updated this revision to Diff 517372. chaitanyav added a comment. Add parentheses around integer expressions in libcxx tests Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D147844/new/ https://reviews.llvm.org/D147844 Files:

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

chaitanyav updated this revision to Diff 517300. chaitanyav added a comment. Revert to earliest patch Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D147844/new/ https://reviews.llvm.org/D147844 Files: clang/docs/ReleaseNotes.rst

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

chaitanyav added a comment. In D147844#4299874 , @aaron.ballman wrote: > In D147844#4299856 , @chaitanyav > wrote: > >> Disable precedence conditional warning by default; Revert changes to test >> files > >

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

aaron.ballman added a comment. 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

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

chaitanyav updated this revision to Diff 517268. chaitanyav added a comment. Disable precedence conditional warning by default; Revert changes to test files Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D147844/new/ https://reviews.llvm.org/D147844

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

aaron.ballman added a comment. 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

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

chaitanyav added a comment. @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 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D147844/new/

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

cjdb added a comment. In D147844#4293988 , @dblaikie wrote: > In D147844#4293743 , @cjdb wrote: > >> In D147844#4293693 , @dblaikie >> wrote: >> I think some of the

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

aaron.ballman added a comment. 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 -

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

chaitanyav updated this revision to Diff 517070. chaitanyav added a comment. Herald added a subscriber: arphaman. Fix more failing tests due to missing parentheses in conditional operator expression Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

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

chaitanyav added a comment. Added parentheses to lots of files to fix the precedence warning. Still a lot to change in OpenMP. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D147844/new/ https://reviews.llvm.org/D147844

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

chaitanyav updated this revision to Diff 517042. chaitanyav added a comment. Herald added subscribers: jplehr, kosarev, jdoerfert, sstefan1, kerbowa, jvesely. Herald added a reviewer: jdoerfert. Fix tests/code by adding parentheses around the conditional operator expression Repository: rG

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

chaitanyav added a comment. 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

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

dblaikie added a comment. 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

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

cjdb added a comment. 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

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

cor3ntin added a comment. 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

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

dblaikie added a comment. > 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

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

cjdb added a comment. 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. Comment at: clang/test/Sema/parentheses.c:94 (void)(x + y > 0 ? 1 : 2);

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

aaron.ballman added reviewers: dblaikie, cjdb, echristo, clang-language-wg. aaron.ballman added a comment. 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

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

Mordante added a comment. 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. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D147844/new/