[clang-tidy] Fix broken fix-its with bugprone-not-null-terminated-result on Windows
#162874
+7
−14
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
localspook
requested review from
5chmidti,
EugeneZelenko,
HerrCai0907,
PiotrZSL,
carlosgalvezp and
vbvictor
|
@llvm/pr-subscribers-clang-tidy Author: Victor Chernyakin (localspook) Changes(See the test changes for a description of the problem.) This is the problematic code: The key thing to know is that, on Windows, Consider the Implicit conversions strike again! (While we're at it: this 64-bit result is stored in a Full diff: https://github.com/llvm/llvm-project/pull/162874.diff 4 Files Affected:
diff --git a/clang-tools-extra/clang-tidy/bugprone/NotNullTerminatedResultCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/NotNullTerminatedResultCheck.cpp
index d4676842a97ff..39e3a542aaf6b 100644
--- a/clang-tools-extra/clang-tidy/bugprone/NotNullTerminatedResultCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/NotNullTerminatedResultCheck.cpp
@@ -307,10 +307,9 @@ static void lengthExprHandle(const Expr *LengthExpr,
// Try to obtain an 'IntegerLiteral' and adjust it.
if (!IsMacroDefinition) {
if (const auto *LengthIL = dyn_cast<IntegerLiteral>(LengthExpr)) {
- size_t NewLength = LengthIL->getValue().getZExtValue() +
- (LengthHandle == LengthHandleKind::Increase
- ? (isInjectUL(Result) ? 1UL : 1)
- : -1);
+ uint64_t NewLength =
+ LengthIL->getValue().getZExtValue() +
+ (LengthHandle == LengthHandleKind::Increase ? 1 : -1);
const auto NewLengthFix = FixItHint::CreateReplacement(
LengthIL->getSourceRange(),
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index d2d79dcc92ec2..f0b21b88dd998 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -274,6 +274,10 @@ Changes in existing checks
<clang-tidy/checks/bugprone/narrowing-conversions>` check by fixing
false positive from analysis of a conditional expression in C.
+- Improved :doc:`bugprone-not-null-terminated-result
+ <clang-tidy/checks/bugprone/not-null-terminated-result>` check by fixing
+ bogus fix-its for `strncmp` and `wcsncmp` on Windows.
+
- Improved :doc:`bugprone-reserved-identifier
<clang-tidy/checks/bugprone/reserved-identifier>` check by ignoring
declarations and macros in system headers.
diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/not-null-terminated-result-strlen.c b/clang-tools-extra/test/clang-tidy/checkers/bugprone/not-null-terminated-result-strlen.c
index 366c1698e4f2d..795ab3d652f9f 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/bugprone/not-null-terminated-result-strlen.c
+++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/not-null-terminated-result-strlen.c
@@ -1,11 +1,6 @@
// RUN: %check_clang_tidy --match-partial-fixes %s bugprone-not-null-terminated-result %t -- \
// RUN: -- -I %S/Inputs/not-null-terminated-result
-// FIXME: Something wrong with the APInt un/signed conversion on Windows:
-// in 'strncmp(str6, "string", 7);' it tries to inject '4294967302' as length.
-
-// UNSUPPORTED: system-windows
-
#include "not-null-terminated-result-c.h"
#define __STDC_LIB_EXT1__ 1
diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/not-null-terminated-result-wcslen.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/not-null-terminated-result-wcslen.cpp
index 06e2db9d6e0d6..288c5faabb0ec 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/bugprone/not-null-terminated-result-wcslen.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/not-null-terminated-result-wcslen.cpp
@@ -1,11 +1,6 @@
// RUN: %check_clang_tidy --match-partial-fixes %s bugprone-not-null-terminated-result %t -- \
// RUN: -- -std=c++11 -I %S/Inputs/not-null-terminated-result
-// FIXME: Something wrong with the APInt un/signed conversion on Windows:
-// in 'wcsncmp(wcs6, L"string", 7);' it tries to inject '4294967302' as length.
-
-// UNSUPPORTED: system-windows
-
#include "not-null-terminated-result-cxx.h"
#define __STDC_LIB_EXT1__ 1
|
|
@llvm/pr-subscribers-clang-tools-extra Author: Victor Chernyakin (localspook) Changes(See the test changes for a description of the problem.) This is the problematic code: The key thing to know is that, on Windows, Consider the Implicit conversions strike again! (While we're at it: this 64-bit result is stored in a Full diff: https://github.com/llvm/llvm-project/pull/162874.diff 4 Files Affected:
diff --git a/clang-tools-extra/clang-tidy/bugprone/NotNullTerminatedResultCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/NotNullTerminatedResultCheck.cpp
index d4676842a97ff..39e3a542aaf6b 100644
--- a/clang-tools-extra/clang-tidy/bugprone/NotNullTerminatedResultCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/NotNullTerminatedResultCheck.cpp
@@ -307,10 +307,9 @@ static void lengthExprHandle(const Expr *LengthExpr,
// Try to obtain an 'IntegerLiteral' and adjust it.
if (!IsMacroDefinition) {
if (const auto *LengthIL = dyn_cast<IntegerLiteral>(LengthExpr)) {
- size_t NewLength = LengthIL->getValue().getZExtValue() +
- (LengthHandle == LengthHandleKind::Increase
- ? (isInjectUL(Result) ? 1UL : 1)
- : -1);
+ uint64_t NewLength =
+ LengthIL->getValue().getZExtValue() +
+ (LengthHandle == LengthHandleKind::Increase ? 1 : -1);
const auto NewLengthFix = FixItHint::CreateReplacement(
LengthIL->getSourceRange(),
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index d2d79dcc92ec2..f0b21b88dd998 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -274,6 +274,10 @@ Changes in existing checks
<clang-tidy/checks/bugprone/narrowing-conversions>` check by fixing
false positive from analysis of a conditional expression in C.
+- Improved :doc:`bugprone-not-null-terminated-result
+ <clang-tidy/checks/bugprone/not-null-terminated-result>` check by fixing
+ bogus fix-its for `strncmp` and `wcsncmp` on Windows.
+
- Improved :doc:`bugprone-reserved-identifier
<clang-tidy/checks/bugprone/reserved-identifier>` check by ignoring
declarations and macros in system headers.
diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/not-null-terminated-result-strlen.c b/clang-tools-extra/test/clang-tidy/checkers/bugprone/not-null-terminated-result-strlen.c
index 366c1698e4f2d..795ab3d652f9f 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/bugprone/not-null-terminated-result-strlen.c
+++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/not-null-terminated-result-strlen.c
@@ -1,11 +1,6 @@
// RUN: %check_clang_tidy --match-partial-fixes %s bugprone-not-null-terminated-result %t -- \
// RUN: -- -I %S/Inputs/not-null-terminated-result
-// FIXME: Something wrong with the APInt un/signed conversion on Windows:
-// in 'strncmp(str6, "string", 7);' it tries to inject '4294967302' as length.
-
-// UNSUPPORTED: system-windows
-
#include "not-null-terminated-result-c.h"
#define __STDC_LIB_EXT1__ 1
diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/not-null-terminated-result-wcslen.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/not-null-terminated-result-wcslen.cpp
index 06e2db9d6e0d6..288c5faabb0ec 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/bugprone/not-null-terminated-result-wcslen.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/not-null-terminated-result-wcslen.cpp
@@ -1,11 +1,6 @@
// RUN: %check_clang_tidy --match-partial-fixes %s bugprone-not-null-terminated-result %t -- \
// RUN: -- -std=c++11 -I %S/Inputs/not-null-terminated-result
-// FIXME: Something wrong with the APInt un/signed conversion on Windows:
-// in 'wcsncmp(wcs6, L"string", 7);' it tries to inject '4294967302' as length.
-
-// UNSUPPORTED: system-windows
-
#include "not-null-terminated-result-cxx.h"
#define __STDC_LIB_EXT1__ 1
|
Co-authored-by: EugeneZelenko <eugene.zelenko@gmail.com>
vbvictor
reviewed
Oct 10, 2025
clang-tools-extra/test/clang-tidy/checkers/bugprone/not-null-terminated-result-strlen.c
Show resolved
Hide resolved
vbvictor
approved these changes
Oct 12, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
(See the test changes for a description of the problem.)
This is the problematic code:
llvm-project/clang-tools-extra/clang-tidy/bugprone/NotNullTerminatedResultCheck.cpp
Lines 310 to 313 in 424fa83
The key thing to know is that, on Windows,
unsigned longisuint32_t.Consider the
(isInjectUL(Result) ? 1UL : 1)bit. The common type of1UL(uint32_t) and1(int) isuint32_t, so the type of the overall expression isuint32_t. Now consider the bigger(... ? (isInjectUL(Result) ? 1UL : 1) : -1). We just saw that the type of the first expression isuint32_t, and the type of-1isint, so once again, the type of the overall expression isuint32_t. That means the-1becomesUINT32_MAX.getZExtValue(), however, returnsuint64_t, so when we go to add thisUINT32_MAXto it, it's zero-extended, and boom, instead of subtracting 1, we get a very biguint64_tvalue.Implicit conversions strike again!
(While we're at it: this 64-bit result is stored in a
size_t. That's only 32 bits on a 32-bit system, so we have a narrowing conversion. That's rather ugly, so this PR also fixes that.)