[clang-tidy] Fix invalid fixit from modernize-use-ranges for nullptr used with std::unique_ptr #127162

Andrewyuan34 · 2025-02-14T03:50:46Z

This PR fixes issue #124815 by correcting the handling of nullptr with std::unique_ptr in the modernize-use-ranges check.

Changes:

Modified clang-tools-extra/clang-tidy/modernize/UseRangesCheck.cpp:
- Updated the logic to suppress warnings for nullptr in std::find.
Added unit tests:
- Added new test cases to verify no warning is generated for nullptr.
- Ensured existing behavior for std::unique_ptr<int>() remains unchanged.

Linked Issue:

Fixes #124815.

Testing:

All existing unit tests pass.
New unit tests have been added to verify the fix.

Note:
This is my first PR to the LLVM project. If there are any issues or areas for improvement, please let me know—I’m happy to make adjustments and learn from your feedback! Thank you for your patience and guidance.

github-actions · 2025-02-14T03:51:02Z

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

llvmbot · 2025-02-14T03:51:30Z

@llvm/pr-subscribers-clang-tools-extra

@llvm/pr-subscribers-clang-tidy

Author: None (Andrewyuan34)

Changes

This PR fixes issue #124815 by correcting the handling of nullptr with std::unique_ptr in the modernize-use-ranges check.

Changes:

Modified clang-tools-extra/clang-tidy/modernize/UseRangesCheck.cpp:
- Updated the logic to suppress warnings for nullptr in std::find.
Added unit tests:
- Added new test cases to verify no warning is generated for nullptr.
- Ensured existing behavior for std::unique_ptr<int>() remains unchanged.

Linked Issue:

Fixes #12345.

Testing:

All existing unit tests pass.
New unit tests have been added to verify the fix.

Full diff: https://github.com/llvm/llvm-project/pull/127162.diff

2 Files Affected:

(modified) clang-tools-extra/clang-tidy/modernize/UseRangesCheck.cpp (+1)
(modified) clang-tools-extra/test/clang-tidy/checkers/modernize/use-ranges.cpp (+22-3)

diff --git a/clang-tools-extra/clang-tidy/modernize/UseRangesCheck.cpp b/clang-tools-extra/clang-tidy/modernize/UseRangesCheck.cpp
index 604204e762c78..03b4321ceae99 100644
--- a/clang-tools-extra/clang-tidy/modernize/UseRangesCheck.cpp
+++ b/clang-tools-extra/clang-tidy/modernize/UseRangesCheck.cpp
@@ -165,6 +165,7 @@ utils::UseRangesCheck::ReplacerMap UseRangesCheck::getReplacerMap() const {
       Buff.assign({"::std::", Name});
       Result.try_emplace(Buff, Replacer);
     }
+    // auto Diag = diag("1111");
   }
   if (getLangOpts().CPlusPlus23)
     Result.try_emplace(
diff --git a/clang-tools-extra/test/clang-tidy/checkers/modernize/use-ranges.cpp b/clang-tools-extra/test/clang-tidy/checkers/modernize/use-ranges.cpp
index b022efebfdf4d..57ca038f64511 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/modernize/use-ranges.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/modernize/use-ranges.cpp
@@ -1,14 +1,24 @@
-// RUN: %check_clang_tidy -std=c++20 %s modernize-use-ranges %t -- -- -I %S/Inputs/use-ranges/
-// RUN: %check_clang_tidy -std=c++23 %s modernize-use-ranges %t -check-suffixes=,CPP23 -- -I %S/Inputs/use-ranges/
+// RUN: %check_clang_tidy -std=c++20 %s modernize-use-ranges %t -- -- -I %S/Inputs/
+// RUN: %check_clang_tidy -std=c++23 %s modernize-use-ranges %t -check-suffixes=,CPP23 -- -I %S/Inputs/
+// Example: ./check_clang_tidy.py -std=c++20 checkers/modernize/use-ranges.cpp modernize-use-ranges temp.txt -- -- -I ~/llvm-project/clang-tools-extra/test/clang-tidy/checkers/modernize/Inputs/
 
 // CHECK-FIXES: #include <algorithm>
 // CHECK-FIXES-CPP23: #include <numeric>
 // CHECK-FIXES: #include <ranges>
 
-#include "fake_std.h"
+#include "use-ranges/fake_std.h"
+#include "smart-ptr/unique_ptr.h"
 
 void Positives() {
   std::vector<int> I, J;
+
+  // Expect to have no check messages
+  std::find(I.begin(), I.end(), nullptr);
+
+  std::find(I.begin(), I.end(), std::unique_ptr<int>());
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use a ranges version of this algorithm
+  // CHECK-FIXES: std::ranges::find(I, std::unique_ptr<int>());
+
   std::find(I.begin(), I.end(), 0);
   // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use a ranges version of this algorithm
   // CHECK-FIXES: std::ranges::find(I, 0);
@@ -76,6 +86,14 @@ void Positives() {
 
 void Reverse(){
   std::vector<int> I, J;
+  
+  // Expect to have no check messages
+  std::find(I.rbegin(), I.rend(), nullptr);
+
+  std::find(I.rbegin(), I.rend(), std::unique_ptr<int>());
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use a ranges version of this algorithm
+  // CHECK-FIXES: std::ranges::find(std::ranges::reverse_view(I), std::unique_ptr<int>());
+
   std::find(I.rbegin(), I.rend(), 0);
   // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use a ranges version of this algorithm
   // CHECK-FIXES: std::ranges::find(std::ranges::reverse_view(I), 0);
@@ -112,3 +130,4 @@ void Negatives() {
   // Pathological, but probably shouldn't diagnose this
   std::rotate(I.begin(), I.end(), I.end() + 0);
 }
+

PiotrZSL

Short release notes entry about fix would be nice.

clang-tools-extra/clang-tidy/utils/UseRangesCheck.cpp

PiotrZSL · 2025-02-14T05:43:22Z

clang-tools-extra/test/clang-tidy/checkers/modernize/use-ranges.cpp


 void Positives() {
  std::vector<int> I, J;
+


I assume those tests pass due to "fake_std", would be good to use proper vector:

std::vector<std::unique_ptr<int>>

for these tests.

I assume those tests pass due to "fake_std", would be good to use proper vector:

std::vector<std::unique_ptr<int>>

for these tests.

good point! Already applied.

Andrewyuan34 · 2025-02-14T07:24:46Z

Short release notes entry about fix would be nice.

Done

clang-tools-extra/docs/ReleaseNotes.rst

vbvictor · 2025-02-15T20:56:20Z

clang-tools-extra/docs/ReleaseNotes.rst

@@ -109,6 +109,9 @@ Changes in existing checks
 - Improved :doc:`misc-redundant-expression
  <clang-tidy/checks/misc/redundant-expression>` check by providing additional
  examples and fixing some macro related false positives.
+
+- Improved :doc:`modernize-use-ranges
+  <clang-tidy/checks/modernize/use-ranges>` check by updating suppress warnings logic for ``nullptr`` in ``std::find``.


Please make lines no more than 80 characters long (rule applied to all docs).

chrchr-github · 2025-02-23T10:13:12Z

Thanks for addressing this. However I think a more general solution is required (instead of special-casing std::unique_ptr/nullptr), see #124815 (comment)

HerrCai0907

LGTM. since it is just a patch instead of ideal solution for std::equality_comparable_with stuff, could you add some FIXME in code?

Andrewyuan34 · 2025-02-26T01:09:36Z

Thanks for addressing this. However I think a more general solution is required (instead of special-casing std::unique_ptr/nullptr), see #124815 (comment)

Thanks for the comment, I got confused by the following comments, should I just leave a FIXME or use a more general way to solve it. Or maybe we could create a new issue?

Andrewyuan34 · 2025-02-26T01:11:30Z

LGTM. since it is just a patch instead of ideal solution for std::equality_comparable_with stuff, could you add some FIXME in code?

Thanks for the comment! Already added.

HerrCai0907 · 2025-02-26T06:18:37Z

ping me if you want to merge it.

Andrewyuan34 · 2025-02-26T19:48:34Z

ping me if you want to merge it.

Thanks! I'd like to merge it.

github-actions · 2025-02-27T11:32:56Z

✅ With the latest revision this PR passed the C/C++ code formatter.

Andrewyuan34 · 2025-02-27T14:14:26Z

ping me if you want to merge it.

sorry about forgetting to use git-clang-format to check the differences. Now it should work.

…used with std::unique_ptr

Andrewyuan34 · 2025-02-28T18:23:49Z

ping me if you want to merge it.

I‘m sorry for requesting approval again. But I'm really confused that should I make sure my branch is alwasy up-to-date?

vbvictor · 2025-02-28T18:58:31Z

I‘m sorry for requesting approval again. But I'm really confused that should I make sure my branch is alwasy up-to-date?

Rebase from main periodically, at least before merge.
It's just to make sure that there are no merge conflicts.

Andrewyuan34 · 2025-03-02T20:37:04Z

I‘m sorry for requesting approval again. But I'm really confused that should I make sure my branch is alwasy up-to-date?

Rebase from main periodically, at least before merge. It's just to make sure that there are no merge conflicts.

Thanks for the explanation!

Andrewyuan34 · 2025-03-09T03:31:49Z

@HerrCai0907 Sorry for requesting your approval again, I really have no experience sending a PR.
I appreciate your patience and feedback on my changes. Let me know if there's anything else I should improve.
Thanks for your time! 😊

HerrCai0907 · 2025-03-09T12:08:41Z

Thanks for you contributions!

Some tips:

run git-clang-format -f before commit change
actually you don't need to rebase often. but in past several weeks we have a major release and it causes release notes are cleared. in this case, rebase is needed.
After several successfully land, you can request write access. then you can merge it by yourself.

github-actions · 2025-03-09T12:10:16Z

@Andrewyuan34 Congratulations on having your first Pull Request (PR) merged into the LLVM Project!

Your changes will be combined with recent changes from other authors, then tested by our build bots. If there is a problem with a build, you may receive a report in an email or a comment on this PR.

Please check whether problems have been caused by your change specifically, as the builds can include changes from many authors. It is not uncommon for your change to be included in a build that fails due to someone else's changes, or infrastructure issues.

How to do this, and the rest of the post-merge process, is covered in detail here.

If your change does cause a problem, it may be reverted, or you can revert it yourself. This is a normal part of LLVM development. You can fix your changes and open a new PR to merge them again.

If you don't get any reports, no action is required from you. Your changes are working as expected, well done!

Andrewyuan34 · 2025-03-11T01:15:27Z

Thanks for you contributions!

Some tips:

run git-clang-format -f before commit change

actually you don't need to rebase often. but in past several weeks we have a major release and it causes release notes are cleared. in this case, rebase is needed.

After several successfully land, you can request write access. then you can merge it by yourself.

I really appreciate all of the suggestions! Will take those in mind. Looking forward to the day I have write access.

llvmbot added clang-tools-extra clang-tidy labels Feb 14, 2025

Andrewyuan34 force-pushed the andrew/clang-tidy branch from a1dfd7d to 7b73f5d Compare February 14, 2025 04:33

PiotrZSL approved these changes Feb 14, 2025

View reviewed changes

Andrewyuan34 force-pushed the andrew/clang-tidy branch 2 times, most recently from e084f68 to 6d83f55 Compare February 14, 2025 07:23

vbvictor reviewed Feb 14, 2025

View reviewed changes

clang-tools-extra/docs/ReleaseNotes.rst Outdated Show resolved Hide resolved

vbvictor reviewed Feb 14, 2025

View reviewed changes

clang-tools-extra/docs/ReleaseNotes.rst Outdated Show resolved Hide resolved

Andrewyuan34 force-pushed the andrew/clang-tidy branch from dc76c62 to 101ec5e Compare February 15, 2025 06:00

Andrewyuan34 requested a review from vbvictor February 15, 2025 19:27

vbvictor reviewed Feb 15, 2025

View reviewed changes

Andrewyuan34 force-pushed the andrew/clang-tidy branch from a9b76a2 to c6a732c Compare February 15, 2025 21:07

HerrCai0907 reviewed Feb 24, 2025

View reviewed changes

Andrewyuan34 force-pushed the andrew/clang-tidy branch from deeb8cd to 5bbd811 Compare February 26, 2025 01:06

HerrCai0907 approved these changes Feb 26, 2025

View reviewed changes

Andrewyuan34 force-pushed the andrew/clang-tidy branch from a0cbdf6 to 879ef53 Compare February 27, 2025 13:45

Andrewyuan34 force-pushed the andrew/clang-tidy branch 4 times, most recently from 09b809d to cb684ab Compare February 28, 2025 17:41

[clang-tidy] Fix invalid fixit from modernize-use-ranges for nullptr …

f887433

…used with std::unique_ptr

Andrewyuan34 force-pushed the andrew/clang-tidy branch from cb684ab to f887433 Compare February 28, 2025 17:59

HerrCai0907 merged commit 2a3e782 into llvm:main Mar 9, 2025
10 of 12 checks passed

shiltian mentioned this pull request Mar 10, 2025

[AMDGPU] Fix test failures when expensive checks are enabled #130644

Merged

[clang-tidy] Fix invalid fixit from modernize-use-ranges for nullptr used with std::unique_ptr #127162

[clang-tidy] Fix invalid fixit from modernize-use-ranges for nullptr used with std::unique_ptr #127162

Uh oh!

Conversation

Andrewyuan34 commented Feb 14, 2025 • edited by PiotrZSL Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes:

Linked Issue:

Testing:

Uh oh!

github-actions bot commented Feb 14, 2025

Uh oh!

llvmbot commented Feb 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes:

Linked Issue:

Testing:

Uh oh!

PiotrZSL left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

PiotrZSL Feb 14, 2025

Choose a reason for hiding this comment

Uh oh!

Andrewyuan34 Feb 14, 2025

Choose a reason for hiding this comment

Uh oh!

Andrewyuan34 commented Feb 14, 2025

Uh oh!

Uh oh!

Uh oh!

vbvictor Feb 15, 2025

Choose a reason for hiding this comment

Uh oh!

chrchr-github commented Feb 23, 2025

Uh oh!

HerrCai0907 left a comment

Choose a reason for hiding this comment

Uh oh!

Andrewyuan34 commented Feb 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Andrewyuan34 commented Feb 26, 2025

Uh oh!

HerrCai0907 commented Feb 26, 2025

Uh oh!

Andrewyuan34 commented Feb 26, 2025

Uh oh!

github-actions bot commented Feb 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Andrewyuan34 commented Feb 27, 2025

Uh oh!

Andrewyuan34 commented Feb 28, 2025

Uh oh!

vbvictor commented Feb 28, 2025

Uh oh!

Andrewyuan34 commented Mar 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Andrewyuan34 commented Mar 9, 2025

Uh oh!

HerrCai0907 commented Mar 9, 2025

Uh oh!

Uh oh!

github-actions bot commented Mar 9, 2025

Uh oh!

Andrewyuan34 commented Mar 11, 2025

Uh oh!

Uh oh!

Andrewyuan34 commented Feb 14, 2025 •

edited by PiotrZSL

Loading

llvmbot commented Feb 14, 2025 •

edited

Loading

Andrewyuan34 commented Feb 26, 2025 •

edited

Loading

github-actions bot commented Feb 27, 2025 •

edited

Loading

Andrewyuan34 commented Mar 2, 2025 •

edited

Loading