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

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

2023-10-04 Thread Richard Smith - zygoloid via Phabricator via cfe-commits

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D147844/new/

https://reviews.llvm.org/D147844

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2023-10-04 Thread NagaChaitanya Vellanki via Phabricator via cfe-commits

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
  clang/lib/Sema/SemaExpr.cpp
  clang/test/Analysis/uninit-vals.c
  clang/test/Sema/integer-overflow.c
  clang/test/Sema/parentheses.c
  clang/test/Sema/parentheses.cpp
  clang/test/SemaCXX/array-bounds.cpp
  clang/test/SemaCXX/integer-overflow.cpp
  libcxx/include/__chrono/duration.h
  libcxx/include/strstream
  libcxx/test/libcxx/algorithms/nth_element_stability.pass.cpp
  libcxx/test/libcxx/algorithms/partial_sort_stability.pass.cpp
  libcxx/test/libcxx/algorithms/sort_stability.pass.cpp
  libcxx/test/std/containers/unord/unord.map/eq.different_hash.pass.cpp
  libcxx/test/std/containers/unord/unord.multimap/eq.different_hash.pass.cpp
  libcxx/test/std/containers/unord/unord.multiset/eq.different_hash.pass.cpp
  libcxx/test/std/containers/unord/unord.set/eq.different_hash.pass.cpp
  libcxxabi/src/cxa_personality.cpp

Index: libcxxabi/src/cxa_personality.cpp
===
--- libcxxabi/src/cxa_personality.cpp
+++ libcxxabi/src/cxa_personality.cpp
@@ -718,9 +718,7 @@
 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
@@ -832,9 +830,8 @@
 // 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
@@ -1250,10 +1247,9 @@
 {
 const __shim_type_info* excpType =
 static_cast(new_exception_header->exceptionType);
-adjustedPtr =
-__getExceptionClass(_exception_header->unwindHeader) == kOurDependentExceptionClass ?
-((__cxa_dependent_exception*)new_exception_header)->primaryException :
-new_exception_header + 1;
+adjustedPtr = (__getExceptionClass(_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))
Index: libcxx/test/std/containers/unord/unord.set/eq.different_hash.pass.cpp
===
--- libcxx/test/std/containers/unord/unord.set/eq.different_hash.pass.cpp
+++ libcxx/test/std/containers/unord/unord.set/eq.different_hash.pass.cpp
@@ -40,7 +40,7 @@
 }
 template 
 std::size_t hash_even(T val) {
-  return val & 1 ? 1 : 0;
+  return (val & 1) ? 1 : 0;
 }
 template 
 std::size_t hash_same(T /*val*/) {
@@ -61,7 +61,7 @@
 }
 template 
 size_t hash_even(T* val) {
-  return *val & 1 ? 1 : 0;
+  return (*val & 1) ? 1 : 0;
 }
 
 template 
Index: libcxx/test/std/containers/unord/unord.multiset/eq.different_hash.pass.cpp
===
--- libcxx/test/std/containers/unord/unord.multiset/eq.different_hash.pass.cpp
+++ libcxx/test/std/containers/unord/unord.multiset/eq.different_hash.pass.cpp
@@ -40,7 +40,7 @@
 }
 template 
 std::size_t hash_even(T val) {
-  return val & 1 ? 1 : 0;
+  return (val & 1) ? 1 : 0;
 }
 template 
 std::size_t hash_same(T /*val*/) {
@@ -61,7 +61,7 @@
 }
 template 
 std::size_t hash_even(T* val) {
-  return *val & 1 ? 1 : 0;
+  return (*val & 1) ? 1 : 0;
 }
 
 template 
Index: libcxx/test/std/containers/unord/unord.multimap/eq.different_hash.pass.cpp
===
--- libcxx/test/std/containers/unord/unord.multimap/eq.different_hash.pass.cpp
+++ libcxx/test/std/containers/unord/unord.multimap/eq.different_hash.pass.cpp
@@ -41,7 +41,7 @@
 }
 template 
 std::size_t

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

2023-10-02 Thread Erich Keane via Phabricator via cfe-commits

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
  https://reviews.llvm.org/D147844/new/

https://reviews.llvm.org/D147844

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2023-09-30 Thread NagaChaitanya Vellanki via Phabricator via cfe-commits

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
  https://reviews.llvm.org/D147844/new/

https://reviews.llvm.org/D147844

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2023-09-30 Thread NagaChaitanya Vellanki via Phabricator via cfe-commits

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
  clang/lib/Sema/SemaExpr.cpp
  clang/test/Analysis/uninit-vals.c
  clang/test/Sema/integer-overflow.c
  clang/test/Sema/parentheses.c
  clang/test/Sema/parentheses.cpp
  clang/test/SemaCXX/array-bounds.cpp
  clang/test/SemaCXX/integer-overflow.cpp
  libcxx/include/__chrono/duration.h
  libcxx/include/strstream
  libcxx/test/libcxx/algorithms/nth_element_stability.pass.cpp
  libcxx/test/libcxx/algorithms/partial_sort_stability.pass.cpp
  libcxx/test/libcxx/algorithms/sort_stability.pass.cpp
  libcxx/test/std/containers/unord/unord.map/eq.different_hash.pass.cpp
  libcxx/test/std/containers/unord/unord.multimap/eq.different_hash.pass.cpp
  libcxx/test/std/containers/unord/unord.multiset/eq.different_hash.pass.cpp
  libcxx/test/std/containers/unord/unord.set/eq.different_hash.pass.cpp
  libcxxabi/src/cxa_personality.cpp

Index: libcxxabi/src/cxa_personality.cpp
===
--- libcxxabi/src/cxa_personality.cpp
+++ libcxxabi/src/cxa_personality.cpp
@@ -718,9 +718,7 @@
 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
@@ -832,9 +830,8 @@
 // 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
@@ -1250,10 +1247,9 @@
 {
 const __shim_type_info* excpType =
 static_cast(new_exception_header->exceptionType);
-adjustedPtr =
-__getExceptionClass(_exception_header->unwindHeader) == kOurDependentExceptionClass ?
-((__cxa_dependent_exception*)new_exception_header)->primaryException :
-new_exception_header + 1;
+adjustedPtr = (__getExceptionClass(_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))
Index: libcxx/test/std/containers/unord/unord.set/eq.different_hash.pass.cpp
===
--- libcxx/test/std/containers/unord/unord.set/eq.different_hash.pass.cpp
+++ libcxx/test/std/containers/unord/unord.set/eq.different_hash.pass.cpp
@@ -40,7 +40,7 @@
 }
 template 
 std::size_t hash_even(T val) {
-  return val & 1 ? 1 : 0;
+  return (val & 1) ? 1 : 0;
 }
 template 
 std::size_t hash_same(T /*val*/) {
@@ -61,7 +61,7 @@
 }
 template 
 size_t hash_even(T* val) {
-  return *val & 1 ? 1 : 0;
+  return (*val & 1) ? 1 : 0;
 }
 
 template 
Index: libcxx/test/std/containers/unord/unord.multiset/eq.different_hash.pass.cpp
===
--- libcxx/test/std/containers/unord/unord.multiset/eq.different_hash.pass.cpp
+++ libcxx/test/std/containers/unord/unord.multiset/eq.different_hash.pass.cpp
@@ -40,7 +40,7 @@
 }
 template 
 std::size_t hash_even(T val) {
-  return val & 1 ? 1 : 0;
+  return (val & 1) ? 1 : 0;
 }
 template 
 std::size_t hash_same(T /*val*/) {
@@ -61,7 +61,7 @@
 }
 template 
 std::size_t hash_even(T* val) {
-  return *val & 1 ? 1 : 0;
+  return (*val & 1) ? 1 : 0;
 }
 
 template 
Index: libcxx/test/std/containers/unord/unord.multimap/eq.different_hash.pass.cpp
===
--- libcxx/test/std/containers/unord/unord.multimap/eq.different_hash.pass.cpp
+++ libcxx/test/std/containers/unord/unord.multimap/eq.different_hash.pass.cpp
@@ -41,7 +41,7 @@
 }
 template 
 std::size_t

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

2023-09-27 Thread NagaChaitanya Vellanki via Phabricator via cfe-commits

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:
  clang/docs/ReleaseNotes.rst
  clang/lib/Sema/SemaExpr.cpp
  clang/test/Analysis/uninit-vals.c
  clang/test/Sema/integer-overflow.c
  clang/test/Sema/parentheses.c
  clang/test/Sema/parentheses.cpp
  clang/test/SemaCXX/array-bounds.cpp
  clang/test/SemaCXX/integer-overflow.cpp
  libcxx/include/__chrono/duration.h
  libcxx/include/strstream
  libcxx/test/libcxx/algorithms/nth_element_stability.pass.cpp
  libcxx/test/libcxx/algorithms/partial_sort_stability.pass.cpp
  libcxx/test/libcxx/algorithms/sort_stability.pass.cpp
  libcxx/test/std/containers/unord/unord.map/eq.different_hash.pass.cpp
  libcxx/test/std/containers/unord/unord.multimap/eq.different_hash.pass.cpp
  libcxx/test/std/containers/unord/unord.multiset/eq.different_hash.pass.cpp
  libcxx/test/std/containers/unord/unord.set/eq.different_hash.pass.cpp
  libcxxabi/src/cxa_personality.cpp

Index: libcxxabi/src/cxa_personality.cpp
===
--- libcxxabi/src/cxa_personality.cpp
+++ libcxxabi/src/cxa_personality.cpp
@@ -718,9 +718,7 @@
 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
@@ -832,9 +830,8 @@
 // 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
@@ -1250,10 +1247,9 @@
 {
 const __shim_type_info* excpType =
 static_cast(new_exception_header->exceptionType);
-adjustedPtr =
-__getExceptionClass(_exception_header->unwindHeader) == kOurDependentExceptionClass ?
-((__cxa_dependent_exception*)new_exception_header)->primaryException :
-new_exception_header + 1;
+adjustedPtr = (__getExceptionClass(_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))
Index: libcxx/test/std/containers/unord/unord.set/eq.different_hash.pass.cpp
===
--- libcxx/test/std/containers/unord/unord.set/eq.different_hash.pass.cpp
+++ libcxx/test/std/containers/unord/unord.set/eq.different_hash.pass.cpp
@@ -40,7 +40,7 @@
 }
 template 
 std::size_t hash_even(T val) {
-  return val & 1 ? 1 : 0;
+  return (val & 1) ? 1 : 0;
 }
 template 
 std::size_t hash_same(T /*val*/) {
@@ -61,7 +61,7 @@
 }
 template 
 size_t hash_even(T* val) {
-  return *val & 1 ? 1 : 0;
+  return (*val & 1) ? 1 : 0;
 }
 
 template 
Index: libcxx/test/std/containers/unord/unord.multiset/eq.different_hash.pass.cpp
===
--- libcxx/test/std/containers/unord/unord.multiset/eq.different_hash.pass.cpp
+++ libcxx/test/std/containers/unord/unord.multiset/eq.different_hash.pass.cpp
@@ -40,7 +40,7 @@
 }
 template 
 std::size_t hash_even(T val) {
-  return val & 1 ? 1 : 0;
+  return (val & 1) ? 1 : 0;
 }
 template 
 std::size_t hash_same(T /*val*/) {
@@ -61,7 +61,7 @@
 }
 template 
 std::size_t hash_even(T* val) {
-  return *val & 1 ? 1 : 0;
+  return (*val & 1) ? 1 : 0;
 }
 
 template 
Index: libcxx/test/std/containers/unord/unord.multimap/eq.different_hash.pass.cpp
===
--- libcxx/test/std/containers/unord/unord.multimap/eq.different_hash.pass.cpp
+++ libcxx/test/std/containers/unord/unord.multimap/eq.different_hash.pass.cpp
@@ -41,7 +41,7 @@
 }

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

2023-09-27 Thread NagaChaitanya Vellanki via Phabricator via cfe-commits

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:
  clang/docs/ReleaseNotes.rst
  clang/lib/Sema/SemaExpr.cpp
  clang/test/Analysis/uninit-vals.c
  clang/test/Sema/integer-overflow.c
  clang/test/Sema/parentheses.c
  clang/test/Sema/parentheses.cpp
  clang/test/SemaCXX/array-bounds.cpp
  clang/test/SemaCXX/integer-overflow.cpp
  libcxx/include/__chrono/duration.h
  libcxx/include/strstream
  libcxx/test/libcxx/algorithms/nth_element_stability.pass.cpp
  libcxx/test/libcxx/algorithms/partial_sort_stability.pass.cpp
  libcxx/test/libcxx/algorithms/sort_stability.pass.cpp
  libcxx/test/std/containers/unord/unord.multiset/eq.different_hash.pass.cpp
  libcxxabi/src/cxa_personality.cpp

Index: libcxxabi/src/cxa_personality.cpp
===
--- libcxxabi/src/cxa_personality.cpp
+++ libcxxabi/src/cxa_personality.cpp
@@ -718,9 +718,7 @@
 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
@@ -832,9 +830,8 @@
 // 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
@@ -1250,10 +1247,9 @@
 {
 const __shim_type_info* excpType =
 static_cast(new_exception_header->exceptionType);
-adjustedPtr =
-__getExceptionClass(_exception_header->unwindHeader) == kOurDependentExceptionClass ?
-((__cxa_dependent_exception*)new_exception_header)->primaryException :
-new_exception_header + 1;
+adjustedPtr = (__getExceptionClass(_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))
Index: libcxx/test/std/containers/unord/unord.multiset/eq.different_hash.pass.cpp
===
--- libcxx/test/std/containers/unord/unord.multiset/eq.different_hash.pass.cpp
+++ libcxx/test/std/containers/unord/unord.multiset/eq.different_hash.pass.cpp
@@ -40,7 +40,7 @@
 }
 template 
 std::size_t hash_even(T val) {
-  return val & 1 ? 1 : 0;
+  return (val & 1) ? 1 : 0;
 }
 template 
 std::size_t hash_same(T /*val*/) {
@@ -61,7 +61,7 @@
 }
 template 
 std::size_t hash_even(T* val) {
-  return *val & 1 ? 1 : 0;
+  return (*val & 1) ? 1 : 0;
 }
 
 template 
Index: libcxx/test/libcxx/algorithms/sort_stability.pass.cpp
===
--- libcxx/test/libcxx/algorithms/sort_stability.pass.cpp
+++ libcxx/test/libcxx/algorithms/sort_stability.pass.cpp
@@ -32,7 +32,7 @@
   std::vector 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 comp;
   std::__sort_dispatch(v.begin(), v.end(), comp);
@@ -44,7 +44,7 @@
   std::vector 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());
@@ -62,7 +62,7 @@
   std::vector 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;
Index: libcxx/test/libcxx/algorithms/partial_sort_stability.pass.cpp

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

2023-09-27 Thread Erich Keane via Phabricator via cfe-commits

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D147844/new/

https://reviews.llvm.org/D147844

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2023-09-26 Thread NagaChaitanya Vellanki via Phabricator via cfe-commits

chaitanyav added a comment.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D147844/new/

https://reviews.llvm.org/D147844

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2023-09-26 Thread NagaChaitanya Vellanki via Phabricator via cfe-commits

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
  clang/lib/Sema/SemaExpr.cpp
  clang/test/Analysis/uninit-vals.c
  clang/test/Sema/integer-overflow.c
  clang/test/Sema/parentheses.c
  clang/test/Sema/parentheses.cpp
  clang/test/SemaCXX/array-bounds.cpp
  clang/test/SemaCXX/integer-overflow.cpp
  libcxx/include/__chrono/duration.h
  libcxx/include/strstream
  libcxx/test/libcxx/algorithms/nth_element_stability.pass.cpp
  libcxx/test/libcxx/algorithms/partial_sort_stability.pass.cpp
  libcxx/test/libcxx/algorithms/sort_stability.pass.cpp
  libcxxabi/src/cxa_personality.cpp

Index: libcxxabi/src/cxa_personality.cpp
===
--- libcxxabi/src/cxa_personality.cpp
+++ libcxxabi/src/cxa_personality.cpp
@@ -718,9 +718,7 @@
 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
@@ -832,9 +830,8 @@
 // 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
@@ -1250,10 +1247,9 @@
 {
 const __shim_type_info* excpType =
 static_cast(new_exception_header->exceptionType);
-adjustedPtr =
-__getExceptionClass(_exception_header->unwindHeader) == kOurDependentExceptionClass ?
-((__cxa_dependent_exception*)new_exception_header)->primaryException :
-new_exception_header + 1;
+adjustedPtr = (__getExceptionClass(_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))
Index: libcxx/test/libcxx/algorithms/sort_stability.pass.cpp
===
--- libcxx/test/libcxx/algorithms/sort_stability.pass.cpp
+++ libcxx/test/libcxx/algorithms/sort_stability.pass.cpp
@@ -32,7 +32,7 @@
   std::vector 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 comp;
   std::__sort_dispatch(v.begin(), v.end(), comp);
@@ -44,7 +44,7 @@
   std::vector 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());
@@ -62,7 +62,7 @@
   std::vector 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;
Index: libcxx/test/libcxx/algorithms/partial_sort_stability.pass.cpp
===
--- libcxx/test/libcxx/algorithms/partial_sort_stability.pass.cpp
+++ libcxx/test/libcxx/algorithms/partial_sort_stability.pass.cpp
@@ -32,7 +32,7 @@
   std::vector 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();
   std::__partial_sort_impl(v.begin(), v.begin() + kSize / 2, v.end(), comp);
@@ -44,7 +44,7 @@
   std::vector 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();

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

2023-09-26 Thread Aaron Ballman via Phabricator via cfe-commits

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 ``_Nullable`` involving alias

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D147844/new/

https://reviews.llvm.org/D147844

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2023-09-25 Thread NagaChaitanya Vellanki via Phabricator via cfe-commits

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:
  clang/docs/ReleaseNotes.rst
  clang/lib/Sema/SemaExpr.cpp
  clang/test/Analysis/uninit-vals.c
  clang/test/Sema/integer-overflow.c
  clang/test/Sema/parentheses.c
  clang/test/Sema/parentheses.cpp
  clang/test/SemaCXX/array-bounds.cpp
  clang/test/SemaCXX/integer-overflow.cpp
  libcxx/include/__chrono/duration.h
  libcxx/include/strstream
  libcxx/test/libcxx/algorithms/nth_element_stability.pass.cpp
  libcxx/test/libcxx/algorithms/partial_sort_stability.pass.cpp
  libcxx/test/libcxx/algorithms/sort_stability.pass.cpp
  libcxxabi/src/cxa_personality.cpp

Index: libcxxabi/src/cxa_personality.cpp
===
--- libcxxabi/src/cxa_personality.cpp
+++ libcxxabi/src/cxa_personality.cpp
@@ -718,9 +718,7 @@
 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
@@ -832,9 +830,8 @@
 // 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
@@ -1250,10 +1247,9 @@
 {
 const __shim_type_info* excpType =
 static_cast(new_exception_header->exceptionType);
-adjustedPtr =
-__getExceptionClass(_exception_header->unwindHeader) == kOurDependentExceptionClass ?
-((__cxa_dependent_exception*)new_exception_header)->primaryException :
-new_exception_header + 1;
+adjustedPtr = (__getExceptionClass(_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))
Index: libcxx/test/libcxx/algorithms/sort_stability.pass.cpp
===
--- libcxx/test/libcxx/algorithms/sort_stability.pass.cpp
+++ libcxx/test/libcxx/algorithms/sort_stability.pass.cpp
@@ -32,7 +32,7 @@
   std::vector 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 comp;
   std::__sort_dispatch(v.begin(), v.end(), comp);
@@ -44,7 +44,7 @@
   std::vector 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());
@@ -62,7 +62,7 @@
   std::vector 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;
Index: libcxx/test/libcxx/algorithms/partial_sort_stability.pass.cpp
===
--- libcxx/test/libcxx/algorithms/partial_sort_stability.pass.cpp
+++ libcxx/test/libcxx/algorithms/partial_sort_stability.pass.cpp
@@ -32,7 +32,7 @@
   std::vector 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();
   std::__partial_sort_impl(v.begin(), v.begin() + kSize / 2, v.end(), comp);
@@ -44,7 +44,7 @@
   std::vector 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

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

2023-09-19 Thread Fangrui Song via Phabricator via cfe-commits

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

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D147844/new/

https://reviews.llvm.org/D147844

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2023-09-15 Thread Fangrui Song via Phabricator via cfe-commits

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 SINCE LAST ACTION
  https://reviews.llvm.org/D147844/new/

https://reviews.llvm.org/D147844

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2023-09-15 Thread Aaron Ballman via Phabricator via cfe-commits

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D147844/new/

https://reviews.llvm.org/D147844

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2023-09-08 Thread Louis Dionne via Phabricator via cfe-commits

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
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2023-05-24 Thread David Blaikie via Phabricator via cfe-commits

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 diagnostic" I won't stand in the way of it.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D147844/new/

https://reviews.llvm.org/D147844

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2023-05-23 Thread NagaChaitanya Vellanki via Phabricator via cfe-commits

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 new to a codebase). I have uploaded the make logs 
for the projects. I will attach the LLVM log in a bit since am building it from 
scratch. It's taking a while.
F27618552: apache_arrow_cpp_out.txt 

F27618556: apache_trafficserver_out.txt 

F27618559: protobuf_out.txt 

F27618563: folly_out.txt 

F27618565: tensorstore_out.txt 

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



  
[1m/home/nvellanki/scratch/arrow/cpp/src/arrow/vendored/datetime/date.h:3941:61:
 [0m[0;1;35mwarning: [0m[1moperator '?:' has lower precedence than '%'; '%' 
will be evaluated first [-Wparentheses][0m
  static CONSTDATA std::uint64_t value = h * h * (exp % 2 ? 10 : 1);
  [0;1;32m~~~ ^
  
[0m[1m/home/nvellanki/scratch/arrow/cpp/src/arrow/vendored/datetime/date.h:3941:61:
 [0m[0;1;30mnote: [0mplace parentheses around the '%' expression to silence 
this warning[0m
  static CONSTDATA std::uint64_t value = h * h * (exp % 2 ? 10 : 1);
  [0;1;32m^
  [0m[0;32m(  )
  
[0m[1m/home/nvellanki/scratch/arrow/cpp/src/arrow/vendored/datetime/date.h:3941:61:
 [0m[0;1;30mnote: [0mplace parentheses around the '?:' expression to 
evaluate it first[0m
  static CONSTDATA std::uint64_t value = h * h * (exp % 2 ? 10 : 1);
  [0;1;32m^
  [0m[0;32m  (
 )



  
/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(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(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 ?

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

2023-05-23 Thread NagaChaitanya Vellanki via Phabricator via cfe-commits

chaitanyav added a comment.

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.




Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D147844/new/

https://reviews.llvm.org/D147844

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2023-05-22 Thread Aaron Ballman via Phabricator via cfe-commits

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

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D147844/new/

https://reviews.llvm.org/D147844

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2023-05-22 Thread NagaChaitanya Vellanki via Phabricator via cfe-commits

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
  clang/lib/Sema/SemaExpr.cpp
  clang/test/Analysis/uninit-vals.c
  clang/test/Sema/integer-overflow.c
  clang/test/Sema/parentheses.c
  clang/test/Sema/parentheses.cpp
  clang/test/SemaCXX/array-bounds.cpp
  clang/test/SemaCXX/integer-overflow.cpp
  libcxx/include/__chrono/duration.h
  libcxx/include/strstream
  libcxx/test/libcxx/algorithms/nth_element_stability.pass.cpp
  libcxx/test/libcxx/algorithms/partial_sort_stability.pass.cpp
  libcxx/test/libcxx/algorithms/sort_stability.pass.cpp
  libcxxabi/src/cxa_personality.cpp

Index: libcxxabi/src/cxa_personality.cpp
===
--- libcxxabi/src/cxa_personality.cpp
+++ libcxxabi/src/cxa_personality.cpp
@@ -718,9 +718,7 @@
 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
@@ -832,9 +830,8 @@
 // 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
@@ -1243,10 +1240,9 @@
 {
 const __shim_type_info* excpType =
 static_cast(new_exception_header->exceptionType);
-adjustedPtr =
-__getExceptionClass(_exception_header->unwindHeader) == kOurDependentExceptionClass ?
-((__cxa_dependent_exception*)new_exception_header)->primaryException :
-new_exception_header + 1;
+adjustedPtr = (__getExceptionClass(_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))
Index: libcxx/test/libcxx/algorithms/sort_stability.pass.cpp
===
--- libcxx/test/libcxx/algorithms/sort_stability.pass.cpp
+++ libcxx/test/libcxx/algorithms/sort_stability.pass.cpp
@@ -32,7 +32,7 @@
   std::vector 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 comp;
   std::__sort_dispatch(v.begin(), v.end(), comp);
@@ -44,7 +44,7 @@
   std::vector 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());
@@ -62,7 +62,7 @@
   std::vector 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;
Index: libcxx/test/libcxx/algorithms/partial_sort_stability.pass.cpp
===
--- libcxx/test/libcxx/algorithms/partial_sort_stability.pass.cpp
+++ libcxx/test/libcxx/algorithms/partial_sort_stability.pass.cpp
@@ -32,7 +32,7 @@
   std::vector 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();
   std::__partial_sort_impl(v.begin(), v.begin() + kSize / 2, v.end(), comp);
@@ -44,7 +44,7 @@
   std::vector 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();

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

2023-05-22 Thread NagaChaitanya Vellanki via Phabricator via cfe-commits

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
  clang/lib/Sema/SemaExpr.cpp
  clang/test/Analysis/uninit-vals.c
  clang/test/Sema/integer-overflow.c
  clang/test/Sema/parentheses.c
  clang/test/Sema/parentheses.cpp
  clang/test/SemaCXX/array-bounds.cpp
  clang/test/SemaCXX/integer-overflow.cpp
  libcxx/include/__chrono/duration.h
  libcxx/include/strstream
  libcxx/test/libcxx/algorithms/nth_element_stability.pass.cpp
  libcxx/test/libcxx/algorithms/partial_sort_stability.pass.cpp
  libcxx/test/libcxx/algorithms/sort_stability.pass.cpp
  libcxxabi/src/cxa_personality.cpp

Index: libcxxabi/src/cxa_personality.cpp
===
--- libcxxabi/src/cxa_personality.cpp
+++ libcxxabi/src/cxa_personality.cpp
@@ -718,9 +718,7 @@
 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
@@ -832,9 +830,8 @@
 // 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
@@ -1243,10 +1240,9 @@
 {
 const __shim_type_info* excpType =
 static_cast(new_exception_header->exceptionType);
-adjustedPtr =
-__getExceptionClass(_exception_header->unwindHeader) == kOurDependentExceptionClass ?
-((__cxa_dependent_exception*)new_exception_header)->primaryException :
-new_exception_header + 1;
+adjustedPtr = (__getExceptionClass(_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))
Index: libcxx/test/libcxx/algorithms/sort_stability.pass.cpp
===
--- libcxx/test/libcxx/algorithms/sort_stability.pass.cpp
+++ libcxx/test/libcxx/algorithms/sort_stability.pass.cpp
@@ -32,7 +32,7 @@
   std::vector 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 comp;
   std::__sort_dispatch(v.begin(), v.end(), comp);
@@ -44,7 +44,7 @@
   std::vector 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());
@@ -62,7 +62,7 @@
   std::vector 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;
Index: libcxx/test/libcxx/algorithms/partial_sort_stability.pass.cpp
===
--- libcxx/test/libcxx/algorithms/partial_sort_stability.pass.cpp
+++ libcxx/test/libcxx/algorithms/partial_sort_stability.pass.cpp
@@ -32,7 +32,7 @@
   std::vector 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();
   std::__partial_sort_impl(v.begin(), v.begin() + kSize / 2, v.end(), comp);
@@ -44,7 +44,7 @@
   std::vector 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();

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

2023-05-20 Thread NagaChaitanya Vellanki via Phabricator via cfe-commits

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
  clang/lib/Sema/SemaExpr.cpp
  clang/test/Analysis/uninit-vals.c
  clang/test/Sema/integer-overflow.c
  clang/test/Sema/parentheses.c
  clang/test/Sema/parentheses.cpp
  clang/test/SemaCXX/array-bounds.cpp
  clang/test/SemaCXX/integer-overflow.cpp
  libcxx/include/__chrono/duration.h
  libcxx/include/strstream
  libcxx/test/libcxx/algorithms/nth_element_stability.pass.cpp
  libcxx/test/libcxx/algorithms/partial_sort_stability.pass.cpp
  libcxx/test/libcxx/algorithms/sort_stability.pass.cpp
  libcxxabi/src/cxa_personality.cpp

Index: libcxxabi/src/cxa_personality.cpp
===
--- libcxxabi/src/cxa_personality.cpp
+++ libcxxabi/src/cxa_personality.cpp
@@ -718,9 +718,7 @@
 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
@@ -832,9 +830,8 @@
 // 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
@@ -1243,10 +1240,9 @@
 {
 const __shim_type_info* excpType =
 static_cast(new_exception_header->exceptionType);
-adjustedPtr =
-__getExceptionClass(_exception_header->unwindHeader) == kOurDependentExceptionClass ?
-((__cxa_dependent_exception*)new_exception_header)->primaryException :
-new_exception_header + 1;
+adjustedPtr = (__getExceptionClass(_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))
Index: libcxx/test/libcxx/algorithms/sort_stability.pass.cpp
===
--- libcxx/test/libcxx/algorithms/sort_stability.pass.cpp
+++ libcxx/test/libcxx/algorithms/sort_stability.pass.cpp
@@ -32,7 +32,7 @@
   std::vector 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 comp;
   std::__sort_dispatch(v.begin(), v.end(), comp);
@@ -44,7 +44,7 @@
   std::vector 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());
@@ -62,7 +62,7 @@
   std::vector 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;
Index: libcxx/test/libcxx/algorithms/partial_sort_stability.pass.cpp
===
--- libcxx/test/libcxx/algorithms/partial_sort_stability.pass.cpp
+++ libcxx/test/libcxx/algorithms/partial_sort_stability.pass.cpp
@@ -32,7 +32,7 @@
   std::vector 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();
   std::__partial_sort_impl(v.begin(), v.begin() + kSize / 2, v.end(), comp);
@@ -44,7 +44,7 @@
   std::vector 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();

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

2023-05-11 Thread David Blaikie via Phabricator via cfe-commits

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

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D147844/new/

https://reviews.llvm.org/D147844

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2023-05-11 Thread Aaron Ballman via Phabricator via cfe-commits

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 @ldionne! My plan is to accept & land this if we hear no objections 
by next Thur (May 18).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D147844/new/

https://reviews.llvm.org/D147844

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2023-05-11 Thread NagaChaitanya Vellanki via Phabricator via cfe-commits

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
  clang/lib/Sema/SemaExpr.cpp
  clang/test/Analysis/uninit-vals.c
  clang/test/Sema/integer-overflow.c
  clang/test/Sema/parentheses.c
  clang/test/Sema/parentheses.cpp
  clang/test/SemaCXX/array-bounds.cpp
  clang/test/SemaCXX/integer-overflow.cpp
  libcxx/include/__chrono/duration.h
  libcxx/include/strstream
  libcxx/test/libcxx/algorithms/nth_element_stability.pass.cpp
  libcxx/test/libcxx/algorithms/partial_sort_stability.pass.cpp
  libcxx/test/libcxx/algorithms/sort_stability.pass.cpp
  libcxxabi/src/cxa_personality.cpp

Index: libcxxabi/src/cxa_personality.cpp
===
--- libcxxabi/src/cxa_personality.cpp
+++ libcxxabi/src/cxa_personality.cpp
@@ -718,9 +718,7 @@
 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
@@ -832,9 +830,8 @@
 // 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
@@ -1243,10 +1240,9 @@
 {
 const __shim_type_info* excpType =
 static_cast(new_exception_header->exceptionType);
-adjustedPtr =
-__getExceptionClass(_exception_header->unwindHeader) == kOurDependentExceptionClass ?
-((__cxa_dependent_exception*)new_exception_header)->primaryException :
-new_exception_header + 1;
+adjustedPtr = (__getExceptionClass(_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))
Index: libcxx/test/libcxx/algorithms/sort_stability.pass.cpp
===
--- libcxx/test/libcxx/algorithms/sort_stability.pass.cpp
+++ libcxx/test/libcxx/algorithms/sort_stability.pass.cpp
@@ -32,7 +32,7 @@
   std::vector 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 comp;
   std::__sort_dispatch(v.begin(), v.end(), comp);
@@ -44,7 +44,7 @@
   std::vector 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());
@@ -62,7 +62,7 @@
   std::vector 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;
Index: libcxx/test/libcxx/algorithms/partial_sort_stability.pass.cpp
===
--- libcxx/test/libcxx/algorithms/partial_sort_stability.pass.cpp
+++ libcxx/test/libcxx/algorithms/partial_sort_stability.pass.cpp
@@ -32,7 +32,7 @@
   std::vector 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();
   std::__partial_sort_impl(v.begin(), v.begin() + kSize / 2, v.end(), comp);
@@ -44,7 +44,7 @@
   std::vector 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();

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

2023-05-10 Thread NagaChaitanya Vellanki via Phabricator via cfe-commits

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
  clang/lib/Sema/SemaExpr.cpp
  clang/test/Analysis/uninit-vals.c
  clang/test/Sema/integer-overflow.c
  clang/test/Sema/parentheses.c
  clang/test/Sema/parentheses.cpp
  clang/test/SemaCXX/array-bounds.cpp
  clang/test/SemaCXX/integer-overflow.cpp
  libcxx/include/__chrono/duration.h
  libcxx/include/strstream
  libcxx/test/libcxx/algorithms/nth_element_stability.pass.cpp
  libcxx/test/libcxx/algorithms/partial_sort_stability.pass.cpp
  libcxx/test/libcxx/algorithms/sort_stability.pass.cpp
  libcxxabi/src/cxa_personality.cpp

Index: libcxxabi/src/cxa_personality.cpp
===
--- libcxxabi/src/cxa_personality.cpp
+++ libcxxabi/src/cxa_personality.cpp
@@ -718,9 +718,7 @@
 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
@@ -832,9 +830,8 @@
 // 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
@@ -1243,10 +1240,9 @@
 {
 const __shim_type_info* excpType =
 static_cast(new_exception_header->exceptionType);
-adjustedPtr =
-__getExceptionClass(_exception_header->unwindHeader) == kOurDependentExceptionClass ?
-((__cxa_dependent_exception*)new_exception_header)->primaryException :
-new_exception_header + 1;
+adjustedPtr = (__getExceptionClass(_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))
Index: libcxx/test/libcxx/algorithms/sort_stability.pass.cpp
===
--- libcxx/test/libcxx/algorithms/sort_stability.pass.cpp
+++ libcxx/test/libcxx/algorithms/sort_stability.pass.cpp
@@ -32,7 +32,7 @@
   std::vector 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 comp;
   std::__sort_dispatch(v.begin(), v.end(), comp);
@@ -44,7 +44,7 @@
   std::vector 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());
@@ -62,7 +62,7 @@
   std::vector 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;
Index: libcxx/test/libcxx/algorithms/partial_sort_stability.pass.cpp
===
--- libcxx/test/libcxx/algorithms/partial_sort_stability.pass.cpp
+++ libcxx/test/libcxx/algorithms/partial_sort_stability.pass.cpp
@@ -32,7 +32,7 @@
   std::vector 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();
   std::__partial_sort_impl(v.begin(), v.begin() + kSize / 2, v.end(), comp);
@@ -44,7 +44,7 @@
   std::vector 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();

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

2023-05-10 Thread Louis Dionne via Phabricator via cfe-commits

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.




Comment at: libcxxabi/src/cxa_personality.cpp:721
 // 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;

Double-parens are not required.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D147844/new/

https://reviews.llvm.org/D147844

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2023-05-09 Thread NagaChaitanya Vellanki via Phabricator via cfe-commits

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:
  clang/docs/ReleaseNotes.rst
  clang/lib/Sema/SemaExpr.cpp
  clang/test/Analysis/uninit-vals.c
  clang/test/Sema/integer-overflow.c
  clang/test/Sema/parentheses.c
  clang/test/Sema/parentheses.cpp
  clang/test/SemaCXX/array-bounds.cpp
  clang/test/SemaCXX/integer-overflow.cpp
  libcxx/include/__chrono/duration.h
  libcxx/include/strstream
  libcxx/test/libcxx/algorithms/nth_element_stability.pass.cpp
  libcxx/test/libcxx/algorithms/partial_sort_stability.pass.cpp
  libcxx/test/libcxx/algorithms/sort_stability.pass.cpp
  libcxxabi/src/cxa_personality.cpp

Index: libcxxabi/src/cxa_personality.cpp
===
--- libcxxabi/src/cxa_personality.cpp
+++ libcxxabi/src/cxa_personality.cpp
@@ -718,9 +718,7 @@
 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
@@ -832,9 +830,8 @@
 // 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
@@ -1243,10 +1240,9 @@
 {
 const __shim_type_info* excpType =
 static_cast(new_exception_header->exceptionType);
-adjustedPtr =
-__getExceptionClass(_exception_header->unwindHeader) == kOurDependentExceptionClass ?
-((__cxa_dependent_exception*)new_exception_header)->primaryException :
-new_exception_header + 1;
+adjustedPtr = (__getExceptionClass(_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))
Index: libcxx/test/libcxx/algorithms/sort_stability.pass.cpp
===
--- libcxx/test/libcxx/algorithms/sort_stability.pass.cpp
+++ libcxx/test/libcxx/algorithms/sort_stability.pass.cpp
@@ -32,7 +32,7 @@
   std::vector 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 comp;
   std::__sort_dispatch(v.begin(), v.end(), comp);
@@ -44,7 +44,7 @@
   std::vector 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());
@@ -62,7 +62,7 @@
   std::vector 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;
Index: libcxx/test/libcxx/algorithms/partial_sort_stability.pass.cpp
===
--- libcxx/test/libcxx/algorithms/partial_sort_stability.pass.cpp
+++ libcxx/test/libcxx/algorithms/partial_sort_stability.pass.cpp
@@ -32,7 +32,7 @@
   std::vector 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();
   std::__partial_sort_impl(v.begin(), v.begin() + kSize / 2, v.end(), comp);
@@ -44,7 +44,7 @@
   std::vector 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);
   }

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

2023-05-09 Thread Aaron Ballman via Phabricator via cfe-commits

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




Comment at: clang/docs/ReleaseNotes.rst:374-375
   (`#62305 `_)
+  (`#62305 `_)
+- Print diagnostic warning about precedence when integer expression is used
+  without parentheses in an conditional operator expression

It looks like a rebase accidentally duplicated this line.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D147844/new/

https://reviews.llvm.org/D147844

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2023-05-08 Thread NagaChaitanya Vellanki via Phabricator via cfe-commits

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
  clang/lib/Sema/SemaExpr.cpp
  clang/test/Analysis/uninit-vals.c
  clang/test/Sema/integer-overflow.c
  clang/test/Sema/parentheses.c
  clang/test/Sema/parentheses.cpp
  clang/test/SemaCXX/array-bounds.cpp
  clang/test/SemaCXX/integer-overflow.cpp
  libcxx/include/__chrono/duration.h
  libcxx/include/strstream
  libcxx/test/libcxx/algorithms/nth_element_stability.pass.cpp
  libcxx/test/libcxx/algorithms/partial_sort_stability.pass.cpp
  libcxx/test/libcxx/algorithms/sort_stability.pass.cpp
  libcxxabi/src/cxa_personality.cpp

Index: libcxxabi/src/cxa_personality.cpp
===
--- libcxxabi/src/cxa_personality.cpp
+++ libcxxabi/src/cxa_personality.cpp
@@ -718,9 +718,7 @@
 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
@@ -832,9 +830,8 @@
 // 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
@@ -1243,10 +1240,9 @@
 {
 const __shim_type_info* excpType =
 static_cast(new_exception_header->exceptionType);
-adjustedPtr =
-__getExceptionClass(_exception_header->unwindHeader) == kOurDependentExceptionClass ?
-((__cxa_dependent_exception*)new_exception_header)->primaryException :
-new_exception_header + 1;
+adjustedPtr = (__getExceptionClass(_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))
Index: libcxx/test/libcxx/algorithms/sort_stability.pass.cpp
===
--- libcxx/test/libcxx/algorithms/sort_stability.pass.cpp
+++ libcxx/test/libcxx/algorithms/sort_stability.pass.cpp
@@ -32,7 +32,7 @@
   std::vector 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 comp;
   std::__sort_dispatch(v.begin(), v.end(), comp);
@@ -44,7 +44,7 @@
   std::vector 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());
@@ -62,7 +62,7 @@
   std::vector 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;
Index: libcxx/test/libcxx/algorithms/partial_sort_stability.pass.cpp
===
--- libcxx/test/libcxx/algorithms/partial_sort_stability.pass.cpp
+++ libcxx/test/libcxx/algorithms/partial_sort_stability.pass.cpp
@@ -32,7 +32,7 @@
   std::vector 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();
   std::__partial_sort_impl(v.begin(), v.begin() + kSize / 2, v.end(), comp);
@@ -44,7 +44,7 @@
   std::vector 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();

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

2023-05-03 Thread NagaChaitanya Vellanki via Phabricator via cfe-commits

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
  clang/lib/Sema/SemaExpr.cpp
  clang/test/Analysis/uninit-vals.c
  clang/test/Sema/integer-overflow.c
  clang/test/Sema/parentheses.c
  clang/test/Sema/parentheses.cpp
  clang/test/SemaCXX/array-bounds.cpp
  clang/test/SemaCXX/integer-overflow.cpp
  libcxx/include/__chrono/duration.h
  libcxx/include/strstream
  libcxx/test/libcxx/algorithms/nth_element_stability.pass.cpp
  libcxx/test/libcxx/algorithms/partial_sort_stability.pass.cpp
  libcxx/test/libcxx/algorithms/sort_stability.pass.cpp
  libcxxabi/src/cxa_personality.cpp

Index: libcxxabi/src/cxa_personality.cpp
===
--- libcxxabi/src/cxa_personality.cpp
+++ libcxxabi/src/cxa_personality.cpp
@@ -718,9 +718,7 @@
 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
@@ -832,9 +830,8 @@
 // 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
@@ -1243,10 +1240,9 @@
 {
 const __shim_type_info* excpType =
 static_cast(new_exception_header->exceptionType);
-adjustedPtr =
-__getExceptionClass(_exception_header->unwindHeader) == kOurDependentExceptionClass ?
-((__cxa_dependent_exception*)new_exception_header)->primaryException :
-new_exception_header + 1;
+adjustedPtr = (__getExceptionClass(_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))
Index: libcxx/test/libcxx/algorithms/sort_stability.pass.cpp
===
--- libcxx/test/libcxx/algorithms/sort_stability.pass.cpp
+++ libcxx/test/libcxx/algorithms/sort_stability.pass.cpp
@@ -32,7 +32,7 @@
   std::vector 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 comp;
   std::__sort_dispatch(v.begin(), v.end(), comp);
@@ -44,7 +44,7 @@
   std::vector 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());
@@ -62,7 +62,7 @@
   std::vector 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;
Index: libcxx/test/libcxx/algorithms/partial_sort_stability.pass.cpp
===
--- libcxx/test/libcxx/algorithms/partial_sort_stability.pass.cpp
+++ libcxx/test/libcxx/algorithms/partial_sort_stability.pass.cpp
@@ -32,7 +32,7 @@
   std::vector 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();
   std::__partial_sort_impl(v.begin(), v.begin() + kSize / 2, v.end(), comp);
@@ -44,7 +44,7 @@
   std::vector 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();

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

2023-04-28 Thread NagaChaitanya Vellanki via Phabricator via cfe-commits

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
  clang/lib/Sema/SemaExpr.cpp
  clang/test/Analysis/uninit-vals.c
  clang/test/Sema/integer-overflow.c
  clang/test/Sema/parentheses.c
  clang/test/Sema/parentheses.cpp
  clang/test/SemaCXX/array-bounds.cpp
  clang/test/SemaCXX/integer-overflow.cpp
  libcxx/include/__chrono/duration.h
  libcxx/include/strstream
  libcxx/test/libcxx/algorithms/nth_element_stability.pass.cpp
  libcxx/test/libcxx/algorithms/partial_sort_stability.pass.cpp
  libcxx/test/libcxx/algorithms/sort_stability.pass.cpp
  libcxxabi/src/cxa_personality.cpp

Index: libcxxabi/src/cxa_personality.cpp
===
--- libcxxabi/src/cxa_personality.cpp
+++ libcxxabi/src/cxa_personality.cpp
@@ -718,9 +718,7 @@
 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
@@ -832,9 +830,8 @@
 // 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
@@ -1243,10 +1240,9 @@
 {
 const __shim_type_info* excpType =
 static_cast(new_exception_header->exceptionType);
-adjustedPtr =
-__getExceptionClass(_exception_header->unwindHeader) == kOurDependentExceptionClass ?
-((__cxa_dependent_exception*)new_exception_header)->primaryException :
-new_exception_header + 1;
+adjustedPtr = (__getExceptionClass(_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))
Index: libcxx/test/libcxx/algorithms/sort_stability.pass.cpp
===
--- libcxx/test/libcxx/algorithms/sort_stability.pass.cpp
+++ libcxx/test/libcxx/algorithms/sort_stability.pass.cpp
@@ -32,7 +32,7 @@
   std::vector 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 comp;
   std::__sort_dispatch(v.begin(), v.end(), comp);
@@ -44,7 +44,7 @@
   std::vector 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());
@@ -62,7 +62,7 @@
   std::vector 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;
Index: libcxx/test/libcxx/algorithms/partial_sort_stability.pass.cpp
===
--- libcxx/test/libcxx/algorithms/partial_sort_stability.pass.cpp
+++ libcxx/test/libcxx/algorithms/partial_sort_stability.pass.cpp
@@ -32,7 +32,7 @@
   std::vector 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();
   std::__partial_sort_impl(v.begin(), v.begin() + kSize / 2, v.end(), comp);
@@ -44,7 +44,7 @@
   std::vector 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();

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

2023-04-26 Thread NagaChaitanya Vellanki via Phabricator via cfe-commits

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:
  clang/docs/ReleaseNotes.rst
  clang/lib/Sema/SemaExpr.cpp
  clang/test/Analysis/uninit-vals.c
  clang/test/Sema/integer-overflow.c
  clang/test/Sema/parentheses.c
  clang/test/Sema/parentheses.cpp
  clang/test/SemaCXX/array-bounds.cpp
  clang/test/SemaCXX/integer-overflow.cpp
  libcxx/include/__chrono/duration.h
  libcxx/include/strstream
  libcxx/test/libcxx/algorithms/nth_element_stability.pass.cpp
  libcxx/test/libcxx/algorithms/partial_sort_stability.pass.cpp
  libcxx/test/libcxx/algorithms/sort_stability.pass.cpp
  libcxxabi/src/cxa_personality.cpp

Index: libcxxabi/src/cxa_personality.cpp
===
--- libcxxabi/src/cxa_personality.cpp
+++ libcxxabi/src/cxa_personality.cpp
@@ -718,9 +718,7 @@
 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
@@ -832,9 +830,8 @@
 // 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
@@ -1243,10 +1240,9 @@
 {
 const __shim_type_info* excpType =
 static_cast(new_exception_header->exceptionType);
-adjustedPtr =
-__getExceptionClass(_exception_header->unwindHeader) == kOurDependentExceptionClass ?
-((__cxa_dependent_exception*)new_exception_header)->primaryException :
-new_exception_header + 1;
+adjustedPtr = (__getExceptionClass(_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))
Index: libcxx/test/libcxx/algorithms/sort_stability.pass.cpp
===
--- libcxx/test/libcxx/algorithms/sort_stability.pass.cpp
+++ libcxx/test/libcxx/algorithms/sort_stability.pass.cpp
@@ -32,7 +32,7 @@
   std::vector 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 comp;
   std::__sort_dispatch(v.begin(), v.end(), comp);
@@ -44,7 +44,7 @@
   std::vector 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());
@@ -62,7 +62,7 @@
   std::vector 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;
Index: libcxx/test/libcxx/algorithms/partial_sort_stability.pass.cpp
===
--- libcxx/test/libcxx/algorithms/partial_sort_stability.pass.cpp
+++ libcxx/test/libcxx/algorithms/partial_sort_stability.pass.cpp
@@ -32,7 +32,7 @@
   std::vector 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();
   std::__partial_sort_impl(v.begin(), v.begin() + kSize / 2, v.end(), comp);
@@ -44,7 +44,7 @@
   std::vector 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

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

2023-04-26 Thread NagaChaitanya Vellanki via Phabricator via cfe-commits

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:
  clang/docs/ReleaseNotes.rst
  clang/lib/Sema/SemaExpr.cpp
  clang/test/Analysis/uninit-vals.c
  clang/test/Sema/integer-overflow.c
  clang/test/Sema/parentheses.c
  clang/test/Sema/parentheses.cpp
  clang/test/SemaCXX/array-bounds.cpp
  clang/test/SemaCXX/integer-overflow.cpp
  libcxx/include/__chrono/duration.h
  libcxx/include/strstream
  libcxx/src/filesystem/operations.cpp
  libcxx/src/locale.cpp
  libcxx/src/memory_resource.cpp
  libcxx/src/ryu/d2fixed.cpp
  libcxx/src/ryu/f2s.cpp
  libcxx/test/libcxx/algorithms/nth_element_stability.pass.cpp
  libcxx/test/libcxx/algorithms/partial_sort_stability.pass.cpp
  libcxx/test/libcxx/algorithms/sort_stability.pass.cpp
  libcxxabi/src/cxa_default_handlers.cpp
  libcxxabi/src/cxa_demangle.cpp
  libcxxabi/src/cxa_exception.cpp
  libcxxabi/src/cxa_personality.cpp
  libcxxabi/src/demangle/ItaniumDemangle.h

Index: libcxxabi/src/demangle/ItaniumDemangle.h
===
--- libcxxabi/src/demangle/ItaniumDemangle.h
+++ libcxxabi/src/demangle/ItaniumDemangle.h
@@ -1306,7 +1306,7 @@
   const Node *getSyntaxNode(OutputBuffer ) const override {
 initializePackExpansion(OB);
 size_t Idx = OB.CurrentPackIndex;
-return Idx < Data.size() ? Data[Idx]->getSyntaxNode(OB) : this;
+return (Idx < Data.size()) ? Data[Idx]->getSyntaxNode(OB) : this;
   }
 
   void printLeft(OutputBuffer ) const override {
@@ -2499,7 +2499,7 @@
 return false;
   }
 
-  char consume() { return First != Last ? *First++ : '\0'; }
+  char consume() { return (First != Last) ? *First++ : '\0'; }
 
   char look(unsigned Lookahead = 0) const {
 if (static_cast(Last - First) <= Lookahead)
Index: libcxxabi/src/cxa_personality.cpp
===
--- libcxxabi/src/cxa_personality.cpp
+++ libcxxabi/src/cxa_personality.cpp
@@ -718,9 +718,7 @@
 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
@@ -832,9 +830,8 @@
 // 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
@@ -1243,10 +1240,9 @@
 {
 const __shim_type_info* excpType =
 static_cast(new_exception_header->exceptionType);
-adjustedPtr =
-__getExceptionClass(_exception_header->unwindHeader) == kOurDependentExceptionClass ?
-((__cxa_dependent_exception*)new_exception_header)->primaryException :
-new_exception_header + 1;
+adjustedPtr = (__getExceptionClass(_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))
Index: libcxxabi/src/cxa_exception.cpp
===
--- libcxxabi/src/cxa_exception.cpp
+++ libcxxabi/src/cxa_exception.cpp
@@ -455,8 +455,8 @@
 if (native_exception)
 {
 // Increment the handler count, removing the flag about being rethrown
-exception_header->handlerCount = exception_header->handlerCount < 0 ?
--exception_header->handlerCount + 1 : exception_header->handlerCount + 1;
+exception_header->handlerCount = (exception_header->handlerCount < 0) ? -exception_header->handlerCount + 1
+  :

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

2023-04-26 Thread NagaChaitanya Vellanki via Phabricator via cfe-commits

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
  clang/lib/Sema/SemaExpr.cpp
  clang/test/Analysis/uninit-vals.c
  clang/test/Sema/integer-overflow.c
  clang/test/Sema/parentheses.c
  clang/test/Sema/parentheses.cpp
  clang/test/SemaCXX/array-bounds.cpp
  clang/test/SemaCXX/integer-overflow.cpp
  libcxx/include/__chrono/duration.h
  libcxx/include/strstream
  libcxx/src/filesystem/operations.cpp
  libcxx/src/locale.cpp
  libcxx/src/memory_resource.cpp
  libcxx/src/ryu/d2fixed.cpp
  libcxx/src/ryu/f2s.cpp
  libcxxabi/src/cxa_default_handlers.cpp
  libcxxabi/src/cxa_demangle.cpp
  libcxxabi/src/cxa_exception.cpp
  libcxxabi/src/cxa_personality.cpp
  libcxxabi/src/demangle/ItaniumDemangle.h

Index: libcxxabi/src/demangle/ItaniumDemangle.h
===
--- libcxxabi/src/demangle/ItaniumDemangle.h
+++ libcxxabi/src/demangle/ItaniumDemangle.h
@@ -1306,7 +1306,7 @@
   const Node *getSyntaxNode(OutputBuffer ) const override {
 initializePackExpansion(OB);
 size_t Idx = OB.CurrentPackIndex;
-return Idx < Data.size() ? Data[Idx]->getSyntaxNode(OB) : this;
+return (Idx < Data.size()) ? Data[Idx]->getSyntaxNode(OB) : this;
   }
 
   void printLeft(OutputBuffer ) const override {
@@ -2499,7 +2499,7 @@
 return false;
   }
 
-  char consume() { return First != Last ? *First++ : '\0'; }
+  char consume() { return (First != Last) ? *First++ : '\0'; }
 
   char look(unsigned Lookahead = 0) const {
 if (static_cast(Last - First) <= Lookahead)
Index: libcxxabi/src/cxa_personality.cpp
===
--- libcxxabi/src/cxa_personality.cpp
+++ libcxxabi/src/cxa_personality.cpp
@@ -718,9 +718,7 @@
 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
@@ -832,9 +830,8 @@
 // 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
@@ -1243,10 +1240,9 @@
 {
 const __shim_type_info* excpType =
 static_cast(new_exception_header->exceptionType);
-adjustedPtr =
-__getExceptionClass(_exception_header->unwindHeader) == kOurDependentExceptionClass ?
-((__cxa_dependent_exception*)new_exception_header)->primaryException :
-new_exception_header + 1;
+adjustedPtr = (__getExceptionClass(_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))
Index: libcxxabi/src/cxa_exception.cpp
===
--- libcxxabi/src/cxa_exception.cpp
+++ libcxxabi/src/cxa_exception.cpp
@@ -455,8 +455,8 @@
 if (native_exception)
 {
 // Increment the handler count, removing the flag about being rethrown
-exception_header->handlerCount = exception_header->handlerCount < 0 ?
--exception_header->handlerCount + 1 : exception_header->handlerCount + 1;
+exception_header->handlerCount = (exception_header->handlerCount < 0) ? -exception_header->handlerCount + 1
+  : exception_header->handlerCount + 1;
 //  place the exception on the top of the stack if it's not already
 //there by a previous rethrow
 if (exception_header != globals->caughtExceptions)
Index:

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

2023-04-26 Thread NagaChaitanya Vellanki via Phabricator via cfe-commits

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D147844/new/

https://reviews.llvm.org/D147844

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2023-04-26 Thread Aaron Ballman via Phabricator via cfe-commits

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

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D147844/new/

https://reviews.llvm.org/D147844

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2023-04-26 Thread NagaChaitanya Vellanki via Phabricator via cfe-commits

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

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaExpr.cpp
  clang/test/Sema/parentheses.c
  clang/test/Sema/parentheses.cpp
  clang/test/Sema/sizeless-1.c
  clang/test/Sema/zvector.c
  clang/test/Sema/zvector2.c
  libcxx/include/__chrono/duration.h
  libcxx/include/strstream
  libcxx/src/filesystem/operations.cpp
  libcxx/src/locale.cpp
  libcxx/src/memory_resource.cpp
  libcxx/src/ryu/d2fixed.cpp
  libcxx/src/ryu/f2s.cpp
  libcxxabi/src/cxa_default_handlers.cpp
  libcxxabi/src/cxa_demangle.cpp
  libcxxabi/src/cxa_exception.cpp
  libcxxabi/src/cxa_personality.cpp
  libcxxabi/src/demangle/ItaniumDemangle.h

Index: libcxxabi/src/demangle/ItaniumDemangle.h
===
--- libcxxabi/src/demangle/ItaniumDemangle.h
+++ libcxxabi/src/demangle/ItaniumDemangle.h
@@ -1306,7 +1306,7 @@
   const Node *getSyntaxNode(OutputBuffer ) const override {
 initializePackExpansion(OB);
 size_t Idx = OB.CurrentPackIndex;
-return Idx < Data.size() ? Data[Idx]->getSyntaxNode(OB) : this;
+return (Idx < Data.size()) ? Data[Idx]->getSyntaxNode(OB) : this;
   }
 
   void printLeft(OutputBuffer ) const override {
@@ -2499,7 +2499,7 @@
 return false;
   }
 
-  char consume() { return First != Last ? *First++ : '\0'; }
+  char consume() { return (First != Last) ? *First++ : '\0'; }
 
   char look(unsigned Lookahead = 0) const {
 if (static_cast(Last - First) <= Lookahead)
Index: libcxxabi/src/cxa_personality.cpp
===
--- libcxxabi/src/cxa_personality.cpp
+++ libcxxabi/src/cxa_personality.cpp
@@ -718,9 +718,7 @@
 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
@@ -832,9 +830,8 @@
 // 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
@@ -1243,10 +1240,9 @@
 {
 const __shim_type_info* excpType =
 static_cast(new_exception_header->exceptionType);
-adjustedPtr =
-__getExceptionClass(_exception_header->unwindHeader) == kOurDependentExceptionClass ?
-((__cxa_dependent_exception*)new_exception_header)->primaryException :
-new_exception_header + 1;
+adjustedPtr = (__getExceptionClass(_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))
Index: libcxxabi/src/cxa_exception.cpp
===
--- libcxxabi/src/cxa_exception.cpp
+++ libcxxabi/src/cxa_exception.cpp
@@ -455,8 +455,8 @@
 if (native_exception)
 {
 // Increment the handler count, removing the flag about being rethrown
-exception_header->handlerCount = exception_header->handlerCount < 0 ?
--exception_header->handlerCount + 1 : exception_header->handlerCount + 1;
+exception_header->handlerCount = (exception_header->handlerCount < 0) ? -exception_header->handlerCount + 1
+  : exception_header->handlerCount + 1;
 //  place the exception on the top of the stack if it's not already
 //there by a previous rethrow
 if (exception_header !=

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

2023-04-26 Thread Aaron Ballman via Phabricator via cfe-commits

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

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D147844/new/

https://reviews.llvm.org/D147844

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2023-04-26 Thread NagaChaitanya Vellanki via Phabricator via cfe-commits

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/

https://reviews.llvm.org/D147844

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2023-04-26 Thread Christopher Di Bella via Phabricator via cfe-commits

cjdb added a comment.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D147844/new/

https://reviews.llvm.org/D147844

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2023-04-26 Thread Aaron Ballman via Phabricator via cfe-commits

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D147844/new/

https://reviews.llvm.org/D147844

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2023-04-26 Thread NagaChaitanya Vellanki via Phabricator via cfe-commits

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
  https://reviews.llvm.org/D147844/new/

https://reviews.llvm.org/D147844

Files:
  clang/docs/ReleaseNotes.rst
  clang/lib/Sema/SemaExpr.cpp
  clang/test/AST/Interp/constexpr-nqueens.cpp
  clang/test/AST/Interp/literals.cpp
  clang/test/Analysis/copypaste/macro-complexity.cpp
  clang/test/Analysis/cxx-uninitialized-object-unguarded-access.cpp
  clang/test/Analysis/malloc-overflow.c
  clang/test/Analysis/misc-ps.m
  clang/test/Analysis/nullability.mm
  clang/test/Analysis/runtime-regression.c
  clang/test/Analysis/uninit-vals.c
  clang/test/Analysis/use-after-move.cpp
  clang/test/CXX/class.access/class.access.dcl/p1.cpp
  clang/test/CXX/drs/dr1xx.cpp
  clang/test/CXX/drs/dr4xx.cpp
  clang/test/CXX/drs/dr5xx.cpp
  clang/test/CXX/drs/dr6xx.cpp
  clang/test/CXX/temp/temp.decls/temp.variadic/partial-ordering.cpp
  clang/test/Frontend/macros.c
  clang/test/Headers/limits.cpp
  clang/test/Index/recursive-cxx-member-calls.cpp
  clang/test/Lexer/char-escapes.c
  clang/test/Lexer/utf8-char-literal.cpp
  clang/test/Misc/no-warn-in-system-macro.c.inc
  clang/test/Modules/module-private.cpp
  clang/test/OpenMP/atomic_messages.c
  clang/test/OpenMP/cancel_if_messages.cpp
  clang/test/OpenMP/distribute_collapse_messages.cpp
  clang/test/OpenMP/distribute_dist_schedule_messages.cpp
  clang/test/OpenMP/distribute_firstprivate_messages.cpp
  clang/test/OpenMP/distribute_parallel_for_collapse_messages.cpp
  clang/test/OpenMP/distribute_parallel_for_copyin_messages.cpp
  clang/test/OpenMP/distribute_parallel_for_dist_schedule_messages.cpp
  clang/test/OpenMP/distribute_parallel_for_firstprivate_messages.cpp
  clang/test/OpenMP/distribute_parallel_for_if_messages.cpp
  clang/test/OpenMP/distribute_parallel_for_lastprivate_messages.cpp
  clang/test/OpenMP/distribute_parallel_for_num_threads_messages.cpp
  clang/test/OpenMP/distribute_parallel_for_private_messages.cpp
  clang/test/OpenMP/distribute_parallel_for_reduction_messages.cpp
  clang/test/OpenMP/distribute_parallel_for_schedule_messages.cpp
  clang/test/OpenMP/distribute_parallel_for_shared_messages.cpp
  clang/test/OpenMP/distribute_parallel_for_simd_aligned_messages.cpp
  clang/test/OpenMP/distribute_parallel_for_simd_collapse_messages.cpp
  clang/test/OpenMP/distribute_parallel_for_simd_copyin_messages.cpp
  clang/test/OpenMP/distribute_parallel_for_simd_dist_schedule_messages.cpp
  clang/test/OpenMP/distribute_parallel_for_simd_firstprivate_messages.cpp
  clang/test/OpenMP/distribute_parallel_for_simd_if_messages.cpp
  clang/test/OpenMP/distribute_parallel_for_simd_lastprivate_messages.cpp
  clang/test/OpenMP/distribute_parallel_for_simd_linear_messages.cpp
  clang/test/OpenMP/distribute_parallel_for_simd_num_threads_messages.cpp
  clang/test/OpenMP/distribute_parallel_for_simd_private_messages.cpp
  clang/test/OpenMP/distribute_parallel_for_simd_reduction_messages.cpp
  clang/test/OpenMP/distribute_parallel_for_simd_safelen_messages.cpp
  clang/test/OpenMP/distribute_parallel_for_simd_schedule_messages.cpp
  clang/test/OpenMP/distribute_parallel_for_simd_shared_messages.cpp
  clang/test/OpenMP/distribute_parallel_for_simd_simdlen_messages.cpp
  clang/test/OpenMP/distribute_private_messages.cpp
  clang/test/OpenMP/distribute_simd_aligned_messages.cpp
  clang/test/OpenMP/distribute_simd_collapse_messages.cpp
  clang/test/OpenMP/distribute_simd_dist_schedule_messages.cpp
  clang/test/OpenMP/distribute_simd_firstprivate_messages.cpp
  clang/test/OpenMP/distribute_simd_if_messages.cpp
  clang/test/OpenMP/distribute_simd_lastprivate_messages.cpp
  clang/test/OpenMP/distribute_simd_linear_messages.cpp
  clang/test/OpenMP/distribute_simd_private_messages.cpp
  clang/test/OpenMP/distribute_simd_reduction_messages.cpp
  clang/test/OpenMP/distribute_simd_safelen_messages.cpp
  clang/test/OpenMP/distribute_simd_simdlen_messages.cpp
  clang/test/OpenMP/for_collapse_messages.cpp
  clang/test/OpenMP/for_firstprivate_messages.cpp
  clang/test/OpenMP/for_lastprivate_messages.cpp
  clang/test/OpenMP/for_linear_messages.cpp
  clang/test/OpenMP/for_ordered_clause.cpp
  clang/test/OpenMP/for_private_messages.cpp
  clang/test/OpenMP/for_reduction_messages.cpp
  clang/test/OpenMP/for_schedule_messages.cpp
  clang/test/OpenMP/for_simd_aligned_messages.cpp
  clang/test/OpenMP/for_simd_collapse_messages.cpp
  clang/test/OpenMP/for_simd_firstprivate_messages.cpp
  clang/test/OpenMP/for_simd_if_messages.cpp
  clang/test/OpenMP/for_simd_lastprivate_messages.cpp
  clang/test/OpenMP/for_simd_linear_messages.cpp
  clang/test/OpenMP/for_simd_private_messages.cpp
  clang/test/OpenMP/for_simd_reduction_messages.cpp
  clang/test/OpenMP/for_simd_safelen_messages.cpp
  clang/test/OpenMP/for_simd_schedule_messages.cpp

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

2023-04-25 Thread NagaChaitanya Vellanki via Phabricator via cfe-commits

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

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2023-04-25 Thread NagaChaitanya Vellanki via Phabricator via cfe-commits

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 LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D147844/new/

https://reviews.llvm.org/D147844

Files:
  clang/docs/ReleaseNotes.rst
  clang/lib/Sema/SemaExpr.cpp
  clang/test/AST/Interp/constexpr-nqueens.cpp
  clang/test/AST/Interp/literals.cpp
  clang/test/Analysis/copypaste/macro-complexity.cpp
  clang/test/Analysis/cxx-uninitialized-object-unguarded-access.cpp
  clang/test/Analysis/malloc-overflow.c
  clang/test/Analysis/misc-ps.m
  clang/test/Analysis/nullability.mm
  clang/test/Analysis/runtime-regression.c
  clang/test/Analysis/uninit-vals.c
  clang/test/Analysis/use-after-move.cpp
  clang/test/CXX/temp/temp.decls/temp.variadic/partial-ordering.cpp
  clang/test/Frontend/macros.c
  clang/test/OpenMP/cancel_if_messages.cpp
  clang/test/OpenMP/distribute_collapse_messages.cpp
  clang/test/OpenMP/distribute_dist_schedule_messages.cpp
  clang/test/OpenMP/distribute_firstprivate_messages.cpp
  clang/test/OpenMP/distribute_parallel_for_collapse_messages.cpp
  clang/test/OpenMP/distribute_parallel_for_copyin_messages.cpp
  clang/test/OpenMP/distribute_parallel_for_dist_schedule_messages.cpp
  clang/test/OpenMP/distribute_parallel_for_firstprivate_messages.cpp
  clang/test/OpenMP/distribute_parallel_for_if_messages.cpp
  clang/test/OpenMP/distribute_parallel_for_lastprivate_messages.cpp
  clang/test/OpenMP/distribute_parallel_for_num_threads_messages.cpp
  clang/test/OpenMP/distribute_parallel_for_private_messages.cpp
  clang/test/OpenMP/distribute_parallel_for_reduction_messages.cpp
  clang/test/OpenMP/distribute_parallel_for_schedule_messages.cpp
  clang/test/OpenMP/distribute_parallel_for_shared_messages.cpp
  clang/test/OpenMP/distribute_parallel_for_simd_aligned_messages.cpp
  clang/test/OpenMP/distribute_parallel_for_simd_collapse_messages.cpp
  clang/test/OpenMP/distribute_parallel_for_simd_copyin_messages.cpp
  clang/test/OpenMP/distribute_parallel_for_simd_dist_schedule_messages.cpp
  clang/test/OpenMP/distribute_parallel_for_simd_firstprivate_messages.cpp
  clang/test/OpenMP/distribute_parallel_for_simd_if_messages.cpp
  clang/test/OpenMP/distribute_parallel_for_simd_lastprivate_messages.cpp
  clang/test/OpenMP/distribute_parallel_for_simd_linear_messages.cpp
  clang/test/OpenMP/distribute_parallel_for_simd_num_threads_messages.cpp
  clang/test/OpenMP/distribute_parallel_for_simd_private_messages.cpp
  clang/test/OpenMP/distribute_parallel_for_simd_reduction_messages.cpp
  clang/test/OpenMP/distribute_parallel_for_simd_safelen_messages.cpp
  clang/test/OpenMP/distribute_parallel_for_simd_schedule_messages.cpp
  clang/test/OpenMP/distribute_parallel_for_simd_shared_messages.cpp
  clang/test/OpenMP/distribute_parallel_for_simd_simdlen_messages.cpp
  clang/test/OpenMP/distribute_private_messages.cpp
  clang/test/OpenMP/distribute_simd_aligned_messages.cpp
  clang/test/OpenMP/distribute_simd_collapse_messages.cpp
  clang/test/OpenMP/distribute_simd_dist_schedule_messages.cpp
  clang/test/OpenMP/distribute_simd_firstprivate_messages.cpp
  clang/test/OpenMP/distribute_simd_if_messages.cpp
  clang/test/OpenMP/distribute_simd_lastprivate_messages.cpp
  clang/test/OpenMP/distribute_simd_linear_messages.cpp
  clang/test/OpenMP/distribute_simd_private_messages.cpp
  clang/test/OpenMP/distribute_simd_reduction_messages.cpp
  clang/test/OpenMP/distribute_simd_safelen_messages.cpp
  clang/test/OpenMP/distribute_simd_simdlen_messages.cpp
  clang/test/OpenMP/for_collapse_messages.cpp
  clang/test/OpenMP/for_firstprivate_messages.cpp
  clang/test/OpenMP/for_lastprivate_messages.cpp
  clang/test/OpenMP/for_linear_messages.cpp
  clang/test/OpenMP/for_ordered_clause.cpp
  clang/test/OpenMP/for_private_messages.cpp
  clang/test/OpenMP/for_reduction_messages.cpp
  clang/test/OpenMP/for_schedule_messages.cpp
  clang/test/OpenMP/for_simd_aligned_messages.cpp
  clang/test/OpenMP/for_simd_collapse_messages.cpp
  clang/test/OpenMP/for_simd_firstprivate_messages.cpp
  clang/test/OpenMP/for_simd_if_messages.cpp
  clang/test/OpenMP/for_simd_lastprivate_messages.cpp
  clang/test/OpenMP/for_simd_linear_messages.cpp
  clang/test/OpenMP/for_simd_private_messages.cpp
  clang/test/OpenMP/for_simd_reduction_messages.cpp
  clang/test/OpenMP/for_simd_safelen_messages.cpp
  clang/test/OpenMP/for_simd_schedule_messages.cpp
  clang/test/OpenMP/for_simd_simdlen_messages.cpp
  clang/test/OpenMP/masked_taskloop_collapse_messages.cpp
  clang/test/OpenMP/masked_taskloop_final_messages.cpp
  clang/test/OpenMP/masked_taskloop_firstprivate_messages.cpp
  clang/test/OpenMP/masked_taskloop_grainsize_messages.cpp
  clang/test/OpenMP/masked_taskloop_in_reduction_messages.cpp

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

2023-04-25 Thread NagaChaitanya Vellanki via Phabricator via cfe-commits

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 quick question, should we restrict to

  >, <, >=, <= 

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




Comment at: clang/test/Sema/parentheses.c:94
 
   (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}}

cjdb wrote:
> I think this should also warn.
@cjdb am looking into 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
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2023-04-24 Thread David Blaikie via Phabricator via cfe-commits

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

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D147844/new/

https://reviews.llvm.org/D147844

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2023-04-24 Thread Christopher Di Bella via Phabricator via cfe-commits

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D147844/new/

https://reviews.llvm.org/D147844

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2023-04-24 Thread Corentin Jabot via Phabricator via cfe-commits

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 curious to see how many time these 
occurs in clang own diagnostics.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D147844/new/

https://reviews.llvm.org/D147844

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2023-04-24 Thread David Blaikie via Phabricator via cfe-commits

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 frequency in a codebase?)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D147844/new/

https://reviews.llvm.org/D147844

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2023-04-24 Thread Christopher Di Bella via Phabricator via cfe-commits

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); // no warning
   (void)(x + (y > 0) ? 1 : 2); // expected-warning {{operator '?:' has lower 
precedence than '+'}} expected-note 2{{place parentheses}}

I think this should also warn.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D147844/new/

https://reviews.llvm.org/D147844

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2023-04-24 Thread Aaron Ballman via Phabricator via cfe-commits

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

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D147844/new/

https://reviews.llvm.org/D147844

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2023-04-22 Thread Mark de Wever via Phabricator via cfe-commits

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/

https://reviews.llvm.org/D147844

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

54 matches

Mail list logo