[PATCH] D71607: [clang-tidy] Add unsigned subtraction warning, with suggestion to convert to unsigned literals.

[Amin Yahyaabadi via Phabricator via cfe-commits]

aminya added a comment.

I just hit this bug in my code where the subtraction of two size_t values 
resulted in a very large value. Fortunately, the address sanitizer immediately 
alerted me about the issue. It would be great to have such a warning in 
clang-tidy.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71607

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

[PATCH] D71607: [clang-tidy] Add unsigned subtraction warning, with suggestion to convert to unsigned literals.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

Mmm, well, on second thought, no, that probably won't be sufficient. As long as 
we're willing to warn on containers about which we know //absolutely nothing//, 
we'll still have some of those false positives. And if we stop warning on such 
containers, the check will probably become much more quiet and much less 
useful. So i think this check will work best as an opt-in style/lint guideline. 
It might still make a bit more sense with path-sensitive analysis but we'll 
still have to accept the inevitable obvious false positives so it's probably 
not worth it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71607



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

[PATCH] D71607: [clang-tidy] Add unsigned subtraction warning, with suggestion to convert to unsigned literals.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

In D71607#2112228 , @MaskRay wrote:

> +@NoQ on comments whether clang static analyzer can catch these cases.
>
> `clang++ --analyze  a.cc` does not warn on `a.size()-2` AFAICT.


Implementing such check in the static analyzer with the help of path-sensitive 
analysis would ultimately allow you to potentially eliminate (in a very precise 
and fairly principled/non-hacky way) false positives such as your example with 
`.empty()` or even this one:

  void foo(vector ) {
v.push_back(a);
v.push_back(b);
  
// Size is known to be at least 2, therefore overflow never occurs.
for (size_t i = 0; i < v.size() - 2; ++i) {
  // ...
}
  }

That won't happen immediately though; it'll require some routine work that'll 
consist in teaching the analyzer facts such as "only empty containers have size 
0" or "vectors grow when pushed into". The analyzer would automagically refute 
such false positives (in all of its checkers!) once it acquires such knowledge. 
That said, it's still a fairly large amount of routine work, so i'd rather not 
have you blocked on this and recommend committing into clang-tidy. We can 
always move or duplicate the check later if you decide to proceed with this 
approach.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71607



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

[PATCH] D71607: [clang-tidy] Add unsigned subtraction warning, with suggestion to convert to unsigned literals.

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added subscribers: NoQ, MaskRay.
MaskRay added a comment.

+@NoQ on comments whether clang static analyzer can catch these cases.

`clang++ --analyze  a.cc` does not warn on `a.size()-2` AFAICT.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71607



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

[PATCH] D71607: [clang-tidy] Add unsigned subtraction warning, with suggestion to convert to unsigned literals.

[Jeffrey Sorensen via Phabricator via cfe-commits]

sorenj added a comment.

My colleague pointed out that -Wsigned-conversion will not detect this very 
frequent mistake

  for (size_t i = 0; i < v.size() - 1; ++i)

It is my contention, and I think it's pretty well substantiated by reviewing 
cases where this detector fails that no coder ever really expects to subtract a 
signed value from an unsigned value.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71607



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

[PATCH] D71607: [clang-tidy] Add unsigned subtraction warning, with suggestion to convert to unsigned literals.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D71607#1828055 , @sorenj wrote:

> Okay, but as you can see the majority of my test cases are intentionally 
> false negatives `- -Wsign-conversion` triggers so often than many people 
> don't use it.


Then we should be addressing that issue rather than duplicating the 
functionality, no?

> And, `unsigned x = 2;` does not trigger a sign conversion warning despite 
> there being a conversion form 2 to 2u.

That should *not* trigger a sign conversion warning because there is no sign 
conversion. We know the exact value of the RHS and can see that it does not 
change signs.

> This check is targeting a very specific but frequent case of functions that 
> do not guard against containers that might be empty.

We should consider a better name for the check then and limit its utility to 
those cases. Truthfully, I think that check would be quite useful, but it would 
almost certainly be a clang static analyzer check to handle control and data 
flow. e.g., such a check should be able to handle these situations:

  size_t count1 = some_container.size() - 1; // Bad if empty
  size_t num_elements = some_container.size();
  size_t count2 = num_elements - 1; // Bad if empty
  if (num_element)
size_t count3 = num_elements - 1; // Just fine
  size_t count4 = std::size(some_container) - 1; // Bad if empty
  size_t count5 = std::distance(some_container.begin(), some_container.end()) - 
1; // Bad if empty? (Note, there's no type-based sign conversion here)
  
  num_elements + 1;
  size_t count6 = num_elements - 1; // Just fine



> Regarding the false positives - I think you are focusing on the semantics of 
> the fix-it which is literally a no-op. The code before and after may be just 
> as wrong, but the salience of the implied conversion is a bit higher. Maybe 
> that's not a good idea for a change that may be applied automatically, even 
> if safe.
> 
> In short I'm not sure if you are objecting the principle here, or the 
> implementation.

A bit of both. I don't think clang-tidy should get a -Wsign-conversion check -- 
the frontend handles that, and if there are deficiencies with that handling, we 
should address those. However, a check that's focused solely on container and 
container-like situations where an empty container would cause problems seems 
like a very interesting check that has value. I'm not certain that implementing 
it in clang-tidy would catch the cases with a reasonable false-positive rate, 
but it seems like it wouldn't be bad as a static analyzer check instead.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71607



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

[PATCH] D71607: [clang-tidy] Add unsigned subtraction warning, with suggestion to convert to unsigned literals.

[Jeffrey Sorensen via Phabricator via cfe-commits]

sorenj added a comment.

So, I ran this check against the cxx directory of llvm, it fired 5 times so 
let's look at the context and disucss:

There are two identical spots in locale.cpp, the first is around line 2717

  uint16_t t = static_cast(
   0xD800
 | wc & 0x1F) >> 16) - 1) << 6)
 |   ((wc & 0x00FC00) >> 10));
   

the fact that a signed value is being right shifted here surprises me, but 
looking earlier there's a branch for wc < 0x01 so we are safe here against 
wc being 0. So this is a false diagnostic. Still, wc is a uint32_t, it's the 
0x1f that converts it to signed. Probably should be 0x1fu? Will still 
false-alarm on this code though unless you use - 1u to make the whole thing 
unsigned end to end.

the third is in memory.cc

  void*
  align(size_t alignment, size_t size, void*& ptr, size_t& space)
  {
  void* r = nullptr;
  if (size <= space)
  {
  char* p1 = static_cast(ptr);
  char* p2 = reinterpret_cast(reinterpret_cast(p1 + 
(alignment - 1)) & -alignment);

here it doesn't make sense for alignment to be zero, and the & -alignment will 
be zero anyway, so false alarm although some check for alignment > 0 might be 
an improvement

The last two are in valarray.cpp lines 35 and 43, I've copied a large excerpt 
here

  void
  gslice::__init(size_t __start)
  {
  valarray __indices(__size_.size());
  size_t __k = __size_.size() != 0;
  for (size_t __i = 0; __i < __size_.size(); ++__i)
  __k *= __size_[__i];
  __1d_.resize(__k);
  if (__1d_.size())
  {
  __k = 0;
  __1d_[__k] = __start;
  while (true)
  {
  size_t __i = __indices.size() - 1;
  while (true)
  {
  if (++__indices[__i] < __size_[__i])
  {
  ++__k;
  __1d_[__k] = __1d_[__k-1] + __stride_[__i];
  for (size_t __j = __i + 1; __j != __indices.size(); ++__j)
  __1d_[__k] -= __stride_[__j] * (__size_[__j] - 1);
  break;
  }
  else
  

with some looking I can see that `__indices.size()` has to be non-zero. It's 
less clear to me that `size_[__j]` is strictly positive here and there should 
probably be some guard against underflow there.

That's a firing rate of about 5/12K lines of code, but I wonder if I were to 
send patches for these 3 files that silence the warning I wonder how many would 
be approved.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71607



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

[PATCH] D71607: [clang-tidy] Add unsigned subtraction warning, with suggestion to convert to unsigned literals.

[Jeffrey Sorensen via Phabricator via cfe-commits]

sorenj added a comment.

Okay, but as you can see the majority of my test cases are intentionally false 
negatives `- -Wsign-conversion` triggers so often than many people don't use 
it. And, `unsigned x = 2;` does not trigger a sign conversion warning despite 
there being a conversion form 2 to 2u. This check is targeting a very specific 
but frequent case of functions that do not guard against containers that might 
be empty.

Regarding the false positives - I think you are focusing on the semantics of 
the fix-it which is literally a no-op. The code before and after may be just as 
wrong, but the salience of the implied conversion is a bit higher. Maybe that's 
not a good idea for a change that may be applied automatically, even if safe.

In short I'm not sure if you are objecting the principle here, or the 
implementation. Is this something that can be refined further? I understand the 
objection in the first case, but what about the second case makes you think 
this is not a good diagnostic and a false positive? It's the specific case I 
was targeting: the unprotected subtraction of a non-zero value from a 
potentially smaller value.

This check was used across our very large code base and was evaluated by 
examining the code by looking at the context where these warning fired. The 
vast majority were judged to be true positives. Unfortunately it's not possible 
to share those results. I will try to run this check on some fraction of the 
llvm code base and follow up.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71607



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

[PATCH] D71607: [clang-tidy] Add unsigned subtraction warning, with suggestion to convert to unsigned literals.

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

Given that the compiler already has `-Wsign-conversion`, I'm not certain what 
value is added by this check. Can you explain a bit about why this is the 
correct approach for diagnosing the issue? When you ignore the false positives 
from the test, the only cases not pointed out by `-Wsign-compare` are the macro 
cases (which is reasonable behavior to not diagnose on -- the fix for one macro 
may make other macros invalid).




Comment at: 
clang-tools-extra/test/clang-tidy/checkers/bugprone-unsigned-subtraction.cpp:36-37
+  if (x - 2 > 0) {
+// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: signed value subtracted from
+// CHECK-FIXES: if (x - 2u > 0) {
+return;

I consider this to be a false positive diagnostic -- the `2` is converted from 
a signed value into an unsigned value and there cannot possibly be a conversion 
error on that implicit cast.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/bugprone-unsigned-subtraction.cpp:100
+  if (y.size() - 1 > 0) {
+// CHECK-MESSAGES: :[[@LINE-1]]:18: warning: signed value subtracted from
+// CHECK-FIXES: if (y.size() - 1u > 0) {

Similarly, I consider this to be a false positive.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71607



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

[PATCH] D71607: [clang-tidy] Add unsigned subtraction warning, with suggestion to convert to unsigned literals.

[Jeffrey Sorensen via Phabricator via cfe-commits]

sorenj added a comment.

Anything further needed?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71607



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

[PATCH] D71607: [clang-tidy] Add unsigned subtraction warning, with suggestion to convert to unsigned literals.

[Jeffrey Sorensen via Phabricator via cfe-commits]

sorenj updated this revision to Diff 237471.
sorenj added a comment.

- Remove double space.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71607

Files:
  clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
  clang-tools-extra/clang-tidy/bugprone/UnsignedSubtractionCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/UnsignedSubtractionCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/bugprone-unsigned-subtraction.rst
  clang-tools-extra/test/clang-tidy/checkers/bugprone-unsigned-subtraction.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-unsigned-subtraction.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-unsigned-subtraction.cpp
@@ -0,0 +1,104 @@
+// RUN: %check_clang_tidy %s bugprone-unsigned-subtraction %t
+
+template 
+class vector {
+ public:
+  unsigned size();
+  bool empty();
+};
+
+#define MACRO_MINUS(x) x - 5
+#define MACRO_INT 20
+
+void signedSubtracted() {
+  unsigned x = 5;
+  int  = 2;
+  if (x -  == 0) {
+// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: signed value subtracted from
+// CHECK-FIXES: if (x - static_cast() == 0) {
+return;
+  }
+  if (0 >= x - ) {
+// CHECK-MESSAGES: :[[@LINE-1]]:16: warning: signed value subtracted from
+// CHECK-FIXES: if (0 >= x - static_cast()) {
+return;
+  }
+  unsigned z = MACRO_MINUS(x);
+  // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: signed value subtracted from
+  z = x - MACRO_INT;
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: signed value subtracted from
+  // CHECK-FIXES: z = x - static_cast(MACRO_INT);
+}
+
+void signedConstantSubtracted() {
+  unsigned x = 5;
+  if (x - 2 > 0) {
+// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: signed value subtracted from
+// CHECK-FIXES: if (x - 2u > 0) {
+return;
+  }
+}
+
+void casesThatShouldBeIgnored() {
+  unsigned x = 5;
+  // If the constant used in subtraction is already explicitly unsigned, do not
+  // warn. This is not safe, but the user presumably knows what they are doing.
+  if (x - 2u > 0u) {
+return;
+  }
+  if (x - 2u > 0) {
+return;
+  }
+  // sizeof operators are strictly positive for all types, and a constexpr, so
+  // any underflow would happen at compile time, so do not warn.
+  x = sizeof(long double) - 1;
+  // If both sides of the subtraction are compile time constants, don't warn.
+  if (5u - 2 > 0) {
+return;
+  }
+  constexpr long y = 4;  // NOLINT(runtime/int)
+  if (y - 4 > 0) {
+return;
+  }
+}
+
+// If the first argument of the subtraction is an expression that was previously
+// used in a comparison, the user is presumed to have done something smart.
+// This isn't perfect, but it greatly reduces false alarms.
+void contextMatters() {
+  unsigned x = 5;
+  if (x < 5) return;
+  if (x - 2 > 0u) {
+return;
+  }
+}
+
+// For loops with a compartor meet the previously described case, and therefore
+// do not warn if the variable is used in a subtraction. Again not perfect, but
+// this code is complex to reason about and it's best not to generate false
+// alarms.
+unsigned forLoops() {
+  unsigned x;
+  for (unsigned i = 1; i < 5; ++i) {
+x += i - 1;
+  }
+  return x;
+}
+
+// Testing for an empty container before subtracting from size() is considered
+// to be the same as comparing against the size - it's still possible to fail
+// but reduces the number of false positives.
+void containersEmpty() {
+  vector x;
+  if (!x.empty()) {
+if (x.size() - 1 > 0) {
+  return;
+}
+  }
+  vector y;
+  if (y.size() - 1 > 0) {
+// CHECK-MESSAGES: :[[@LINE-1]]:18: warning: signed value subtracted from
+// CHECK-FIXES: if (y.size() - 1u > 0) {
+return;
+  }
+}
Index: clang-tools-extra/docs/clang-tidy/checks/bugprone-unsigned-subtraction.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/bugprone-unsigned-subtraction.rst
@@ -0,0 +1,34 @@
+.. title:: clang-tidy - bugprone-unsigned-subtraction
+
+bugprone-sizeof-expression
+==
+
+Finds expressions where a signed value is subtracted from an
+unsigned value, a likely cause of unexpected underflow.
+
+Many programmers are unaware that an expression of unsigned - signed
+will promote the signed argument to unsigned, and if an underflow
+occurs it results in a large positive value. Hence the frequent
+errors related to to tests of ``container.size() - 1 <= 0`` when a
+container is empty.
+
+This check suggest a fix-it change that will append a ``u`` to
+constants, thus making the implict cast explicit and signals that
+the the code was intended. In cases where the second argument is
+not a constant, a ``static_cast`` is recommended. Heuristics are
+employed to reduce warnings in contexts where the subtraction

[PATCH] D71607: [clang-tidy] Add unsigned subtraction warning, with suggestion to convert to unsigned literals.

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/bugprone-unsigned-subtraction.rst:17
+constants, thus making the implict cast explicit and signals that
+the the code was intended.  In cases where the second argument is
+not a constant, a ``static_cast`` is recommended. Heuristics are

Please fix double space.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71607



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

[PATCH] D71607: [clang-tidy] Add unsigned subtraction warning, with suggestion to convert to unsigned literals.

[Jeffrey Sorensen via Phabricator via cfe-commits]

sorenj updated this revision to Diff 237466.
sorenj added a comment.

- Address documentation comments.
- Address documentation comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71607

Files:
  clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
  clang-tools-extra/clang-tidy/bugprone/UnsignedSubtractionCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/UnsignedSubtractionCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/bugprone-unsigned-subtraction.rst
  clang-tools-extra/test/clang-tidy/checkers/bugprone-unsigned-subtraction.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-unsigned-subtraction.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-unsigned-subtraction.cpp
@@ -0,0 +1,104 @@
+// RUN: %check_clang_tidy %s bugprone-unsigned-subtraction %t
+
+template 
+class vector {
+ public:
+  unsigned size();
+  bool empty();
+};
+
+#define MACRO_MINUS(x) x - 5
+#define MACRO_INT 20
+
+void signedSubtracted() {
+  unsigned x = 5;
+  int  = 2;
+  if (x -  == 0) {
+// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: signed value subtracted from
+// CHECK-FIXES: if (x - static_cast() == 0) {
+return;
+  }
+  if (0 >= x - ) {
+// CHECK-MESSAGES: :[[@LINE-1]]:16: warning: signed value subtracted from
+// CHECK-FIXES: if (0 >= x - static_cast()) {
+return;
+  }
+  unsigned z = MACRO_MINUS(x);
+  // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: signed value subtracted from
+  z = x - MACRO_INT;
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: signed value subtracted from
+  // CHECK-FIXES: z = x - static_cast(MACRO_INT);
+}
+
+void signedConstantSubtracted() {
+  unsigned x = 5;
+  if (x - 2 > 0) {
+// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: signed value subtracted from
+// CHECK-FIXES: if (x - 2u > 0) {
+return;
+  }
+}
+
+void casesThatShouldBeIgnored() {
+  unsigned x = 5;
+  // If the constant used in subtraction is already explicitly unsigned, do not
+  // warn. This is not safe, but the user presumably knows what they are doing.
+  if (x - 2u > 0u) {
+return;
+  }
+  if (x - 2u > 0) {
+return;
+  }
+  // sizeof operators are strictly positive for all types, and a constexpr, so
+  // any underflow would happen at compile time, so do not warn.
+  x = sizeof(long double) - 1;
+  // If both sides of the subtraction are compile time constants, don't warn.
+  if (5u - 2 > 0) {
+return;
+  }
+  constexpr long y = 4;  // NOLINT(runtime/int)
+  if (y - 4 > 0) {
+return;
+  }
+}
+
+// If the first argument of the subtraction is an expression that was previously
+// used in a comparison, the user is presumed to have done something smart.
+// This isn't perfect, but it greatly reduces false alarms.
+void contextMatters() {
+  unsigned x = 5;
+  if (x < 5) return;
+  if (x - 2 > 0u) {
+return;
+  }
+}
+
+// For loops with a compartor meet the previously described case, and therefore
+// do not warn if the variable is used in a subtraction. Again not perfect, but
+// this code is complex to reason about and it's best not to generate false
+// alarms.
+unsigned forLoops() {
+  unsigned x;
+  for (unsigned i = 1; i < 5; ++i) {
+x += i - 1;
+  }
+  return x;
+}
+
+// Testing for an empty container before subtracting from size() is considered
+// to be the same as comparing against the size - it's still possible to fail
+// but reduces the number of false positives.
+void containersEmpty() {
+  vector x;
+  if (!x.empty()) {
+if (x.size() - 1 > 0) {
+  return;
+}
+  }
+  vector y;
+  if (y.size() - 1 > 0) {
+// CHECK-MESSAGES: :[[@LINE-1]]:18: warning: signed value subtracted from
+// CHECK-FIXES: if (y.size() - 1u > 0) {
+return;
+  }
+}
Index: clang-tools-extra/docs/clang-tidy/checks/bugprone-unsigned-subtraction.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/bugprone-unsigned-subtraction.rst
@@ -0,0 +1,34 @@
+.. title:: clang-tidy - bugprone-unsigned-subtraction
+
+bugprone-sizeof-expression
+==
+
+Finds expressions where a signed value is subtracted from an
+unsigned value, a likely cause of unexpected underflow.
+
+Many programmers are unaware that an expression of unsigned - signed
+will promote the signed argument to unsigned, and if an underflow
+occurs it results in a large positive value. Hence the frequent
+errors related to to tests of ``container.size() - 1 <= 0`` when a
+container is empty.
+
+This check suggest a fix-it change that will append a ``u`` to
+constants, thus making the implict cast explicit and signals that
+the the code was intended.  In cases where the second argument is
+not a constant, a ``static_cast`` is recommended. Heuristics are
+employed to 

[PATCH] D71607: [clang-tidy] Add unsigned subtraction warning, with suggestion to convert to unsigned literals.

[Jeffrey Sorensen via Phabricator via cfe-commits]

sorenj added a comment.

First time so trying to follow similar recent submits. PTAL.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71607



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

[PATCH] D71607: [clang-tidy] Add unsigned subtraction warning, with suggestion to convert to unsigned literals.

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: clang-tools-extra/docs/ReleaseNotes.rst:106
+
+  Finds cases where a signed value is subtracted from an unsigned value,
+  a likely cause of unexpected underflow.

Please synchronize with first statement in documentation.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/bugprone-unsigned-subtraction.rst:5-6
+==
+
+This check finds expressions where a signed value is subtracted from
+an unsigned value.

This check should be removed.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/bugprone-unsigned-subtraction.rst:15
+
+This warning suggest a fixit change that will append a ``u`` to
+constants, thus making the implict cast explicit and signals that

fix-it. Check, not warning.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/bugprone-unsigned-subtraction.rst:17
+constants, thus making the implict cast explicit and signals that
+the developer intends the subtraction with unsigned arguments.
+In cases where the second argument is not a constant, a

code was intended?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71607



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

[PATCH] D71607: [clang-tidy] Add unsigned subtraction warning, with suggestion to convert to unsigned literals.

[Jeffrey Sorensen via Phabricator via cfe-commits]

sorenj updated this revision to Diff 237440.
sorenj added a comment.

- Merge branch 'master' of https://github.com/llvm/llvm-project
- Add documentation and release notes, fix spelling error.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71607

Files:
  clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
  clang-tools-extra/clang-tidy/bugprone/UnsignedSubtractionCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/UnsignedSubtractionCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/bugprone-unsigned-subtraction.rst
  clang-tools-extra/test/clang-tidy/checkers/bugprone-unsigned-subtraction.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-unsigned-subtraction.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-unsigned-subtraction.cpp
@@ -0,0 +1,104 @@
+// RUN: %check_clang_tidy %s bugprone-unsigned-subtraction %t
+
+template 
+class vector {
+ public:
+  unsigned size();
+  bool empty();
+};
+
+#define MACRO_MINUS(x) x - 5
+#define MACRO_INT 20
+
+void signedSubtracted() {
+  unsigned x = 5;
+  int  = 2;
+  if (x -  == 0) {
+// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: signed value subtracted from
+// CHECK-FIXES: if (x - static_cast() == 0) {
+return;
+  }
+  if (0 >= x - ) {
+// CHECK-MESSAGES: :[[@LINE-1]]:16: warning: signed value subtracted from
+// CHECK-FIXES: if (0 >= x - static_cast()) {
+return;
+  }
+  unsigned z = MACRO_MINUS(x);
+  // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: signed value subtracted from
+  z = x - MACRO_INT;
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: signed value subtracted from
+  // CHECK-FIXES: z = x - static_cast(MACRO_INT);
+}
+
+void signedConstantSubtracted() {
+  unsigned x = 5;
+  if (x - 2 > 0) {
+// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: signed value subtracted from
+// CHECK-FIXES: if (x - 2u > 0) {
+return;
+  }
+}
+
+void casesThatShouldBeIgnored() {
+  unsigned x = 5;
+  // If the constant used in subtraction is already explicitly unsigned, do not
+  // warn. This is not safe, but the user presumably knows what they are doing.
+  if (x - 2u > 0u) {
+return;
+  }
+  if (x - 2u > 0) {
+return;
+  }
+  // sizeof operators are strictly positive for all types, and a constexpr, so
+  // any underflow would happen at compile time, so do not warn.
+  x = sizeof(long double) - 1;
+  // If both sides of the subtraction are compile time constants, don't warn.
+  if (5u - 2 > 0) {
+return;
+  }
+  constexpr long y = 4;  // NOLINT(runtime/int)
+  if (y - 4 > 0) {
+return;
+  }
+}
+
+// If the first argument of the subtraction is an expression that was previously
+// used in a comparison, the user is presumed to have done something smart.
+// This isn't perfect, but it greatly reduces false alarms.
+void contextMatters() {
+  unsigned x = 5;
+  if (x < 5) return;
+  if (x - 2 > 0u) {
+return;
+  }
+}
+
+// For loops with a compartor meet the previously described case, and therefore
+// do not warn if the variable is used in a subtraction. Again not perfect, but
+// this code is complex to reason about and it's best not to generate false
+// alarms.
+unsigned forLoops() {
+  unsigned x;
+  for (unsigned i = 1; i < 5; ++i) {
+x += i - 1;
+  }
+  return x;
+}
+
+// Testing for an empty container before subtracting from size() is considered
+// to be the same as comparing against the size - it's still possible to fail
+// but reduces the number of false positives.
+void containersEmpty() {
+  vector x;
+  if (!x.empty()) {
+if (x.size() - 1 > 0) {
+  return;
+}
+  }
+  vector y;
+  if (y.size() - 1 > 0) {
+// CHECK-MESSAGES: :[[@LINE-1]]:18: warning: signed value subtracted from
+// CHECK-FIXES: if (y.size() - 1u > 0) {
+return;
+  }
+}
Index: clang-tools-extra/docs/clang-tidy/checks/bugprone-unsigned-subtraction.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/bugprone-unsigned-subtraction.rst
@@ -0,0 +1,33 @@
+.. title:: clang-tidy - bugprone-unsigned-subtraction
+
+bugprone-sizeof-expression
+==
+
+This check finds expressions where a signed value is subtracted from
+an unsigned value.
+
+Many programmers are unaware that an expression of unsigned - signed
+will promote the signed argument to unsigned, and if an underflow
+occurs it results in a large positive value. Hence the frequent
+errors related to to tests of ``container.size() - 1 <= 0`` when a
+container is empty.
+
+This warning suggest a fixit change that will append a ``u`` to
+constants, thus making the implict cast explicit and signals that
+the developer intends the subtraction with unsigned arguments.
+In cases where the second argument is not a constant, a

[PATCH] D71607: [clang-tidy] Add unsigned subtraction warning, with suggestion to convert to unsigned literals.

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added a comment.

Documentation and Release Notes entry are still missing..


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71607



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

[PATCH] D71607: [clang-tidy] Add unsigned subtraction warning, with suggestion to convert to unsigned literals.

[Jeffrey Sorensen via Phabricator via cfe-commits]

sorenj added a comment.

Friendly ping - anything further I need to do here?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71607



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

[PATCH] D71607: [clang-tidy] Add unsigned subtraction warning, with suggestion to convert to unsigned literals.

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

Somewhat related: https://reviews.llvm.org/D40854
This is a check that tried to enforce not mixing any signed/unsigned 
arithmetic. there was no feedback from the cppcoreguideline-ppl on how to 
proceed with edge cases and occassion where mixing is not avoidable (e.g. 
`unsigned short + unsigned short + unsigned short`, because of integer 
promotion).
Just to inform you as it might help with testing or anything like that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71607



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

[PATCH] D71607: [clang-tidy] Add unsigned subtraction warning, with suggestion to convert to unsigned literals.

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/UnsignedSubtractionCheck.cpp:21
+
+void UnsignedSubtractionCheck::registerMatchers(MatchFinder *Finder) {
+  const auto UnsignedIntType = hasType(isUnsignedInteger());

sorenj wrote:
> lebedev.ri wrote:
> > The check's name is more generic than what it does - it only looks at mixed 
> > subtractions in comparisons.
> > The name implies it looks at all mixed subtractions.
> But it does look at all mixed subtractions, line 28 of the unit test shows 
> one such example.
Oh, i didn't spot that one, sorry for the false alarm.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71607



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

[PATCH] D71607: [clang-tidy] Add unsigned subtraction warning, with suggestion to convert to unsigned literals.

[Jeffrey Sorensen via Phabricator via cfe-commits]

sorenj marked an inline comment as done.
sorenj added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/UnsignedSubtractionCheck.cpp:21
+
+void UnsignedSubtractionCheck::registerMatchers(MatchFinder *Finder) {
+  const auto UnsignedIntType = hasType(isUnsignedInteger());

lebedev.ri wrote:
> The check's name is more generic than what it does - it only looks at mixed 
> subtractions in comparisons.
> The name implies it looks at all mixed subtractions.
But it does look at all mixed subtractions, line 28 of the unit test shows one 
such example.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71607



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

[PATCH] D71607: [clang-tidy] Add unsigned subtraction warning, with suggestion to convert to unsigned literals.

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/UnsignedSubtractionCheck.cpp:21
+
+void UnsignedSubtractionCheck::registerMatchers(MatchFinder *Finder) {
+  const auto UnsignedIntType = hasType(isUnsignedInteger());

The check's name is more generic than what it does - it only looks at mixed 
subtractions in comparisons.
The name implies it looks at all mixed subtractions.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71607



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

[PATCH] D71607: [clang-tidy] Add unsigned subtraction warning, with suggestion to convert to unsigned literals.

[Jeffrey Sorensen via Phabricator via cfe-commits]

sorenj updated this revision to Diff 234294.
sorenj added a comment.

Address requested whitespace changes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71607

Files:
  clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
  clang-tools-extra/clang-tidy/bugprone/UnsignedSubtractionCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/UnsignedSubtractionCheck.h
  clang-tools-extra/test/clang-tidy/checkers/bugprone-unsigned-subtraction.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-unsigned-subtraction.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-unsigned-subtraction.cpp
@@ -0,0 +1,104 @@
+// RUN: %check_clang_tidy %s bugprone-unsigned-subtraction %t
+
+template 
+class vector {
+ public:
+  unsigned size();
+  bool empty();
+};
+
+#define MACRO_MINUS(x) x - 5
+#define MACRO_INT 20
+
+void signedSubtracted() {
+  unsigned x = 5;
+  int  = 2;
+  if (x -  == 0) {
+// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: signed value subtracted from
+// CHECK-FIXES: if (x - static_cast() == 0) {
+return;
+  }
+  if (0 >= x - ) {
+// CHECK-MESSAGES: :[[@LINE-1]]:16: warning: signed value subtracted from
+// CHECK-FIXES: if (0 >= x - static_cast()) {
+return;
+  }
+  unsigned z = MACRO_MINUS(x);
+  // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: signed value subtracted from
+  z = x - MACRO_INT;
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: signed value subtracted from
+  // CHECK-FIXES: z = x - static_cast(MACRO_INT);
+}
+
+void signedConstantSubtracted() {
+  unsigned x = 5;
+  if (x - 2 > 0) {
+// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: signed value subtracted from
+// CHECK-FIXES: if (x - 2u > 0) {
+return;
+  }
+}
+
+void casesThatShouldBeIgnored() {
+  unsigned x = 5;
+  // If the constant used in subtraction is already explicitly unsigned, do not
+  // warn. This is not safe, but the user presumably knows what they are doing.
+  if (x - 2u > 0u) {
+return;
+  }
+  if (x - 2u > 0) {
+return;
+  }
+  // sizeof operators are strictly positive for all types, and a constexpr, so
+  // any underflow would happen at compile time, so do not warn.
+  x = sizeof(long double) - 1;
+  // If both sides of the subtraction are compile time constants, don't warn.
+  if (5u - 2 > 0) {
+return;
+  }
+  constexpr long y = 4;  // NOLINT(runtime/int)
+  if (y - 4 > 0) {
+return;
+  }
+}
+
+// If the first argument of the subtraction is an expression that was previously
+// used in a comparison, the user is presumed to have done something smart.
+// This isn't perfect, but it greatly reduces false alarms.
+void contextMatters() {
+  unsigned x = 5;
+  if (x < 5) return;
+  if (x - 2 > 0u) {
+return;
+  }
+}
+
+// For loops with a compartor meet the previously described case, and therefore
+// do not warn if the variable is used in a subtraction. Again not perfect, but
+// this code is complex to reason about and it's best not to generate false
+// alarms.
+unsigned forLoops() {
+  unsigned x;
+  for (unsigned i = 1; i < 5; ++i) {
+x += i - 1;
+  }
+  return x;
+}
+
+// Testing for an empty container before subtracting from size() is considered
+// to be the same as comparing against the size - it's still possible to fail
+// but reduces the number of false positives.
+void containersEmpty() {
+  vector x;
+  if (!x.empty()) {
+if (x.size() - 1 > 0) {
+  return;
+}
+  }
+  vector y;
+  if (y.size() - 1 > 0) {
+// CHECK-MESSAGES: :[[@LINE-1]]:18: warning: signed value subtracted from
+// CHECK-FIXES: if (y.size() - 1u > 0) {
+return;
+  }
+}
Index: clang-tools-extra/clang-tidy/bugprone/UnsignedSubtractionCheck.h
===
--- /dev/null
+++ clang-tools-extra/clang-tidy/bugprone/UnsignedSubtractionCheck.h
@@ -0,0 +1,38 @@
+//===--- UnsignedSubtractionCheck.h - clang-tidy *- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_UNSIGNED_SUBTRACTION_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_UNSIGNED_SUBTRACTION_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace bugprone {
+
+/// Finds cases where a signed value is substracted from an unsigned value,
+/// a likely cause of unexpected underflow.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/bugprone-unsigned-subtraction.html
+class UnsignedSubtractionCheck : public ClangTidyCheck {
+ public:
+  

[PATCH] D71607: [clang-tidy] Add unsigned subtraction warning, with suggestion to convert to unsigned literals.

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added a comment.

Please add documentation and mention new check in Release Notes.




Comment at: 
clang-tools-extra/clang-tidy/bugprone/UnsignedSubtractionCheck.cpp:10
+#include "UnsignedSubtractionCheck.h"
+
+#include "../utils/Matchers.h"

Unnecessary empty line.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/UnsignedSubtractionCheck.cpp:137
+}
+}  // namespace bugprone
+}  // namespace tidy

Please separate with empty line.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71607



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