[PATCH] D71607: [clang-tidy] Add unsigned subtraction warning, with suggestion to convert to unsigned literals.
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.
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.
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.
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 redu
[PATCH] D71607: [clang-tidy] Add unsigned subtraction warning, with suggestion to convert to unsigned literals.
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.
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: Add unsigned subtraction warning, with suggestion to convert to unsigned literals.
sorenj created this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
Many C++ programmers are unaware that an expression of unsigned - signed will
promote the signed argument to unsigned, and the resulting underflow produces
a large positive rather than negative result. Hence the frequent errors
related to the test x.size() - 1 <= 0 when the container x is empty.
This clang tidy detects signed values being subtracted from unsigned values
and warns the user about the potential error. It is not perfect as it is
not always possible at compile time to reason about code when this comparison
is made.
The warning also suggests a fixit change that will append a "u" to numerical
constants - this makes the implicit cast explicit and signals that the
developer knew what they were doing in a subtraction. In other cases it
suggests the rather abhorrent static_cast<>().
The easiest fix is to not do subtraction at all, just move the operation
to the other side of the comparison where it becomes an addition - which
has none of these surprising properties.
Repository:
rG LLVM Github Monorepo
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 ---
[PATCH] D71607: [clang-tidy] Add unsigned subtraction warning, with suggestion to convert to unsigned literals.
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:
+ UnsignedSubtractionCheck(String
[PATCH] D71607: [clang-tidy] Add unsigned subtraction warning, with suggestion to convert to unsigned literals.
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.
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.
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.
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
