[NFC] Fixes proposed by code sanitizer. #134138

zahiraam · 2025-04-02T18:53:14Z

No description provided.

llvmbot · 2025-04-03T17:10:08Z

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

Author: Zahira Ammarguellat (zahiraam)

Changes

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

3 Files Affected:

(modified) clang-tools-extra/clang-tidy/bugprone/NarrowingConversionsCheck.cpp (+1-1)
(modified) clang-tools-extra/clangd/ConfigCompile.cpp (+1-1)
(modified) clang-tools-extra/clangd/Headers.cpp (+1-1)

diff --git a/clang-tools-extra/clang-tidy/bugprone/NarrowingConversionsCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/NarrowingConversionsCheck.cpp
index bafcd402ca851..7650d9e8a592f 100644
--- a/clang-tools-extra/clang-tidy/bugprone/NarrowingConversionsCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/NarrowingConversionsCheck.cpp
@@ -260,7 +260,7 @@ static IntegerRange createFromType(const ASTContext &Context,
     llvm::APSInt LowerValue(PrecisionBits + 2, /*isUnsigned*/ false);
     LowerValue.setBit(PrecisionBits);
     LowerValue.setSignBit();
-    return {LowerValue, UpperValue};
+    return {std::move(LowerValue), UpperValue};
   }
   assert(T.isInteger() && "Unexpected builtin type");
   uint64_t TypeSize = Context.getTypeSize(&T);
diff --git a/clang-tools-extra/clangd/ConfigCompile.cpp b/clang-tools-extra/clangd/ConfigCompile.cpp
index 3d7f792aa136b..13c2405e76df7 100644
--- a/clang-tools-extra/clangd/ConfigCompile.cpp
+++ b/clang-tools-extra/clangd/ConfigCompile.cpp
@@ -535,7 +535,7 @@ struct FragmentCompiler {
     }
     if (Filters->empty())
       return std::nullopt;
-    auto Filter = [Filters](llvm::StringRef Path) {
+    auto Filter = [Filters = std::move(Filters)](llvm::StringRef Path) {
       for (auto &Regex : *Filters)
         if (Regex.match(Path))
           return true;
diff --git a/clang-tools-extra/clangd/Headers.cpp b/clang-tools-extra/clangd/Headers.cpp
index 0ffd9ee4d2751..0d43ada87d359 100644
--- a/clang-tools-extra/clangd/Headers.cpp
+++ b/clang-tools-extra/clangd/Headers.cpp
@@ -305,7 +305,7 @@ IncludeInserter::calculateIncludePath(const HeaderFile &InsertedHeader,
   if (llvm::sys::path::is_absolute(Suggested))
     return std::nullopt;
   bool IsAngled = false;
-  for (auto Filter : AngledHeaders) {
+  for (auto &Filter : AngledHeaders) {
     if (Filter(Suggested)) {
       IsAngled = true;
       break;

llvmbot · 2025-04-03T17:10:08Z

@llvm/pr-subscribers-clang-tidy

Author: Zahira Ammarguellat (zahiraam)

Changes

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

3 Files Affected:

(modified) clang-tools-extra/clang-tidy/bugprone/NarrowingConversionsCheck.cpp (+1-1)
(modified) clang-tools-extra/clangd/ConfigCompile.cpp (+1-1)
(modified) clang-tools-extra/clangd/Headers.cpp (+1-1)

diff --git a/clang-tools-extra/clang-tidy/bugprone/NarrowingConversionsCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/NarrowingConversionsCheck.cpp
index bafcd402ca851..7650d9e8a592f 100644
--- a/clang-tools-extra/clang-tidy/bugprone/NarrowingConversionsCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/NarrowingConversionsCheck.cpp
@@ -260,7 +260,7 @@ static IntegerRange createFromType(const ASTContext &Context,
     llvm::APSInt LowerValue(PrecisionBits + 2, /*isUnsigned*/ false);
     LowerValue.setBit(PrecisionBits);
     LowerValue.setSignBit();
-    return {LowerValue, UpperValue};
+    return {std::move(LowerValue), UpperValue};
   }
   assert(T.isInteger() && "Unexpected builtin type");
   uint64_t TypeSize = Context.getTypeSize(&T);
diff --git a/clang-tools-extra/clangd/ConfigCompile.cpp b/clang-tools-extra/clangd/ConfigCompile.cpp
index 3d7f792aa136b..13c2405e76df7 100644
--- a/clang-tools-extra/clangd/ConfigCompile.cpp
+++ b/clang-tools-extra/clangd/ConfigCompile.cpp
@@ -535,7 +535,7 @@ struct FragmentCompiler {
     }
     if (Filters->empty())
       return std::nullopt;
-    auto Filter = [Filters](llvm::StringRef Path) {
+    auto Filter = [Filters = std::move(Filters)](llvm::StringRef Path) {
       for (auto &Regex : *Filters)
         if (Regex.match(Path))
           return true;
diff --git a/clang-tools-extra/clangd/Headers.cpp b/clang-tools-extra/clangd/Headers.cpp
index 0ffd9ee4d2751..0d43ada87d359 100644
--- a/clang-tools-extra/clangd/Headers.cpp
+++ b/clang-tools-extra/clangd/Headers.cpp
@@ -305,7 +305,7 @@ IncludeInserter::calculateIncludePath(const HeaderFile &InsertedHeader,
   if (llvm::sys::path::is_absolute(Suggested))
     return std::nullopt;
   bool IsAngled = false;
-  for (auto Filter : AngledHeaders) {
+  for (auto &Filter : AngledHeaders) {
     if (Filter(Suggested)) {
       IsAngled = true;
       break;

llvmbot · 2025-04-03T17:10:09Z

@llvm/pr-subscribers-clangd

Author: Zahira Ammarguellat (zahiraam)

Changes

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

3 Files Affected:

(modified) clang-tools-extra/clang-tidy/bugprone/NarrowingConversionsCheck.cpp (+1-1)
(modified) clang-tools-extra/clangd/ConfigCompile.cpp (+1-1)
(modified) clang-tools-extra/clangd/Headers.cpp (+1-1)

diff --git a/clang-tools-extra/clang-tidy/bugprone/NarrowingConversionsCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/NarrowingConversionsCheck.cpp
index bafcd402ca851..7650d9e8a592f 100644
--- a/clang-tools-extra/clang-tidy/bugprone/NarrowingConversionsCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/NarrowingConversionsCheck.cpp
@@ -260,7 +260,7 @@ static IntegerRange createFromType(const ASTContext &Context,
     llvm::APSInt LowerValue(PrecisionBits + 2, /*isUnsigned*/ false);
     LowerValue.setBit(PrecisionBits);
     LowerValue.setSignBit();
-    return {LowerValue, UpperValue};
+    return {std::move(LowerValue), UpperValue};
   }
   assert(T.isInteger() && "Unexpected builtin type");
   uint64_t TypeSize = Context.getTypeSize(&T);
diff --git a/clang-tools-extra/clangd/ConfigCompile.cpp b/clang-tools-extra/clangd/ConfigCompile.cpp
index 3d7f792aa136b..13c2405e76df7 100644
--- a/clang-tools-extra/clangd/ConfigCompile.cpp
+++ b/clang-tools-extra/clangd/ConfigCompile.cpp
@@ -535,7 +535,7 @@ struct FragmentCompiler {
     }
     if (Filters->empty())
       return std::nullopt;
-    auto Filter = [Filters](llvm::StringRef Path) {
+    auto Filter = [Filters = std::move(Filters)](llvm::StringRef Path) {
       for (auto &Regex : *Filters)
         if (Regex.match(Path))
           return true;
diff --git a/clang-tools-extra/clangd/Headers.cpp b/clang-tools-extra/clangd/Headers.cpp
index 0ffd9ee4d2751..0d43ada87d359 100644
--- a/clang-tools-extra/clangd/Headers.cpp
+++ b/clang-tools-extra/clangd/Headers.cpp
@@ -305,7 +305,7 @@ IncludeInserter::calculateIncludePath(const HeaderFile &InsertedHeader,
   if (llvm::sys::path::is_absolute(Suggested))
     return std::nullopt;
   bool IsAngled = false;
-  for (auto Filter : AngledHeaders) {
+  for (auto &Filter : AngledHeaders) {
     if (Filter(Suggested)) {
       IsAngled = true;
       break;

PiotrZSL

LGTM

PiotrZSL · 2025-04-03T17:55:06Z

clang-tools-extra/clang-tidy/bugprone/NarrowingConversionsCheck.cpp

@@ -260,7 +260,7 @@ static IntegerRange createFromType(const ASTContext &Context,
    llvm::APSInt LowerValue(PrecisionBits + 2, /*isUnsigned*/ false);
    LowerValue.setBit(PrecisionBits);
    LowerValue.setSignBit();
-    return {LowerValue, UpperValue};
+    return {std::move(LowerValue), UpperValue};


this doesn't have any impact.
And if using move, why not for a second too

Isn't this actually an antipattern? What exactly do you mean by "code sanitizer", can you show the actual error message?

This is the error message:
" LowerValue is passed-by-value as parameter to llvm::APSInt::APSInt(llvm::APSInt const &) /implicit =default/, when it could be moved instead"
"Use std::move(LowerValue) instead of LowerValue."

Do you think this should be reverted?

this doesn't have any impact. And if using move, why not for a second too

Good point. I was only addressing the issues that the sanitizer identified.

What tool did you use to get that error message?

I do not see the point of this change, please revert. We must not blindly do what tools tell us, we must critically think if they make sense or not.

PiotrZSL · 2025-04-03T17:56:54Z

clang-tools-extra/clangd/Headers.cpp

@@ -305,7 +305,7 @@ IncludeInserter::calculateIncludePath(const HeaderFile &InsertedHeader,
  if (llvm::sys::path::is_absolute(Suggested))
    return std::nullopt;
  bool IsAngled = false;
-  for (auto Filter : AngledHeaders) {
+  for (auto &Filter : AngledHeaders) {


same thing in line 315

carlosgalvezp

The changes to the clang-tidy check should be reverted IMO.

Also, please provide more information. What do you mean by "code sanitizer", what is the exact error message, etc.

zahiraam · 2025-04-03T19:26:46Z

The changes to the clang-tidy check should be reverted IMO.

Also, please provide more information. What do you mean by "code sanitizer", what is the exact error message, etc.

Please see above the error message from sanitizer. I can revert the change if you think the don't make sense.

nicovank · 2025-04-03T22:05:12Z

clang-tools-extra/clangd/ConfigCompile.cpp

@@ -535,7 +535,7 @@ struct FragmentCompiler {
    }
    if (Filters->empty())
      return std::nullopt;
-    auto Filter = [Filters](llvm::StringRef Path) {
+    auto Filter = [Filters = std::move(Filters)](llvm::StringRef Path) {


Line 520: this can now be a make_unique.

nicovank · 2025-04-03T22:05:18Z

clang-tools-extra/clangd/ConfigCompile.cpp

@@ -535,7 +535,7 @@ struct FragmentCompiler {
    }
    if (Filters->empty())
      return std::nullopt;
-    auto Filter = [Filters](llvm::StringRef Path) {
+    auto Filter = [Filters = std::move(Filters)](llvm::StringRef Path) {
      for (auto &Regex : *Filters)


Suggested change

for (auto &Regex : *Filters)

for (const auto &Regex : *Filters)

Same for both cases in other file.

carlosgalvezp · 2025-04-04T09:30:31Z

In general, I don't fully understand the scope of this patch. Surely, there's thousands of things that could be improved in the codebase. Which of those belong to this patch? Reviewers can always point out things that could be improved ad infinitum, leading to a patch with lots of unrelated changes. When do we decide that we are done?

zahiraam · 2025-04-04T11:49:36Z

In general, I don't fully understand the scope of this patch. Surely, there's thousands of things that could be improved in the codebase. Which of those belong to this patch? Reviewers can always point out things that could be improved ad infinitum, leading to a patch with lots of unrelated changes. When do we decide that we are done?

I totally agree. Our code sanitizer points us to many edits that are most of the time false positive. I have proposed in this PR fixes that I have judged might be real issues (I might be wrong).
As you suggested I have reverted the changes for clang-tidy. The 2 that are left are in cland/Headers.cpp and clang/ConfigCompile.cpp. If you agree with the edits, please approve. If not I will close the PR.
Thanks.

zahiraam added 2 commits April 2, 2025 11:52

[NFC] Fixes proposed by code sanitizer.

608f582

Removed added file.

5f453eb

zahiraam requested a review from tahonermann April 3, 2025 17:09

Merge branch 'main' into SanitizerFixes

0ab8d53

zahiraam marked this pull request as ready for review April 3, 2025 17:09

llvmbot added clang-tools-extra clangd clang-tidy labels Apr 3, 2025

PiotrZSL approved these changes Apr 3, 2025

View reviewed changes

carlosgalvezp requested changes Apr 3, 2025

View reviewed changes

Added the & at line 315 of Headers.cpp.

c9bfb44

Removed the potential antipattern in clang-tidy.

c51acef

nicovank reviewed Apr 3, 2025

View reviewed changes

carlosgalvezp approved these changes Apr 4, 2025

View reviewed changes

zahiraam merged commit babbc6f into llvm:main Apr 4, 2025
11 checks passed

	for (auto &Regex : *Filters)
	for (const auto &Regex : *Filters)

[NFC] Fixes proposed by code sanitizer. #134138

[NFC] Fixes proposed by code sanitizer. #134138

Uh oh!

Conversation

zahiraam commented Apr 2, 2025

Uh oh!

llvmbot commented Apr 3, 2025

Uh oh!

llvmbot commented Apr 3, 2025

Uh oh!

llvmbot commented Apr 3, 2025

Uh oh!

PiotrZSL left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

carlosgalvezp left a comment

Choose a reason for hiding this comment

Uh oh!

zahiraam commented Apr 3, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

carlosgalvezp commented Apr 4, 2025

Uh oh!

zahiraam commented Apr 4, 2025

Uh oh!

Uh oh!

Uh oh!