[clang-tools-extra] [clangd] Extract to function: Pass unmodified scalar parameters by value (PR #227675)
Christian Kandeler via cfe-commits
cfe-commits at lists.llvm.org
Thu Oct 1 05:22:26 PDT 2026
https://github.com/ckandeler updated https://github.com/llvm/llvm-project/pull/227675
>From 2867fd1217cec89ff8d8dc2f7ca8e71d3a08e8fe Mon Sep 17 00:00:00 2001
From: Christian Kandeler <christian.kandeler at qt.io>
Date: Wed, 30 Sep 2026 13:52:27 +0200
Subject: [PATCH 1/5] [clangd] Extract to function: Pass unmodified scalar
parameters by value
For more natural-looking function signatures.
Assisted-by: Claude
---
.../refactor/tweaks/ExtractFunction.cpp | 30 ++++---
.../unittests/tweaks/ExtractFunctionTests.cpp | 88 +++++++++++++------
2 files changed, 83 insertions(+), 35 deletions(-)
diff --git a/clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp b/clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp
index 171f64737c5dc..3085f237fe10d 100644
--- a/clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp
+++ b/clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp
@@ -25,8 +25,9 @@
// - Only extract statements
// - Extracts from non-templated free functions only.
// - Parameters that are never (conservatively) mutated in the extracted
-// code become const references.
-// - Always passed by l-value reference
+// code become const references, except scalars (arithmetic, pointer,
+// enumeration, ...), which are passed by value instead.
+// - Otherwise passed by non-const reference
// - Void return type
// - Cannot extract declarations that will be needed in the original function
// after extraction.
@@ -992,7 +993,6 @@ CapturedZoneInfo captureZoneInfo(const ExtractionZone &ExtZone) {
// FIXME: Check if the declaration has a local/anonymous type
bool createParameters(NewFunction &ExtractedFunc,
const CapturedZoneInfo &CapturedInfo) {
- // FIXME: Pass non-mutated parameters of built-in type by value.
for (const auto &KeyVal : CapturedInfo.DeclInfoMap) {
const auto &DeclInfo = KeyVal.second;
// If a Decl was Declared in zone and referenced in post zone, it
@@ -1014,15 +1014,25 @@ bool createParameters(NewFunction &ExtractedFunc,
return false;
// Parameter qualifiers are same as the Decl's qualifiers.
QualType TypeInfo = VD->getType().getNonReferenceType();
- // Add const if it's not (conservatively) mutated in the zone: it's
- // still passed by reference to avoid a copy, but the reference doesn't
- // need to be mutable. Array types are never made const: mutating array
- // elements through a non-const-ref loop variable or a decayed pointer
- // argument is common and easy to miss conservatively, so we don't try.
- if (!DeclInfo.IsPossiblyMutated && !TypeInfo->isArrayType())
- TypeInfo.addConst();
// FIXME: check if parameter will be a non l-value reference.
bool IsPassedByReference = true;
+ if (!DeclInfo.IsPossiblyMutated) {
+ // A scalar (arithmetic, pointer, enumeration, ...) is at least as
+ // cheap to copy as to pass by reference, and less noisy. Any
+ // pre-existing const is dropped: it's a no-op on a by-value
+ // parameter, not a signal worth keeping.
+ if (TypeInfo->isScalarType()) {
+ IsPassedByReference = false;
+ TypeInfo.removeLocalConst();
+ } else if (!TypeInfo->isArrayType()) {
+ // Still passed by reference to avoid a copy, but the reference
+ // doesn't need to be mutable. Array types are never made const:
+ // mutating array elements through a non-const-ref loop variable
+ // or a decayed pointer argument is common and easy to miss
+ // conservatively, so we don't try.
+ TypeInfo.addConst();
+ }
+ }
// We use the index of declaration as the ordering priority for parameters.
ExtractedFunc.Parameters.push_back({std::string(VD->getName()), TypeInfo,
IsPassedByReference,
diff --git a/clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp b/clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp
index b43f239b70ffe..7648641c6ffc5 100644
--- a/clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp
+++ b/clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp
@@ -63,8 +63,9 @@ TEST_F(ExtractFunctionTest, FunctionTest) {
TEST_F(ExtractFunctionTest, FileTest) {
// Check all parameters are in order. `a` and `ptr` are mutated in the
- // zone (`+=` and postfix `++` respectively), so stay non-const; `b` and
- // `foo` are only read, so become const references.
+ // zone (`+=` and postfix `++` respectively), so stay non-const; `b` is
+ // an unmutated scalar, so becomes a by-value parameter; `foo` is an
+ // unmutated class type, so becomes a const reference.
std::string ParameterCheckInput = R"cpp(
struct Foo {
int x;
@@ -80,7 +81,7 @@ void f(int a) {
struct Foo {
int x;
};
-void extracted(int &a, const int &b, int * &ptr, const Foo &foo) {
+void extracted(int &a, int b, int * &ptr, const Foo &foo) {
a += foo.x + b;
*ptr++;
}
@@ -92,13 +93,13 @@ void f(int a) {
})cpp";
EXPECT_EQ(apply(ParameterCheckInput), ParameterCheckOutput);
- // Check const qualifier
+ // Check const qualifier: dropped for a by-value scalar.
std::string ConstCheckInput = R"cpp(
void f(const int c) {
[[while(c) {}]]
})cpp";
std::string ConstCheckOutput = R"cpp(
-void extracted(const int &c) {
+void extracted(int c) {
while(c) {}
}
void f(const int c) {
@@ -106,7 +107,7 @@ void f(const int c) {
})cpp";
EXPECT_EQ(apply(ConstCheckInput), ConstCheckOutput);
- // Check const qualifier with namespace
+ // Check const qualifier: kept for a by-reference non-scalar.
std::string ConstNamespaceCheckInput = R"cpp(
namespace X { struct Y { int z; }; }
int f(const X::Y &y) {
@@ -573,11 +574,10 @@ TEST_F(ExtractFunctionTest, ExistingReturnStatement) {
}
}
)cpp";
- // FIXME: min/max should be by value.
// FIXME: avoid emitting redundant braces
const char *After = R"cpp(
bool lucky(int N);
- int extracted(const int &Min, const int &Max) {
+ int extracted(int Min, int Max) {
{
for (int I = Min; I <= Max; ++I)
if (lucky(I))
@@ -782,12 +782,14 @@ TEST_F(ExtractFunctionTest, VarDeclInitializer) {
TEST_F(ExtractFunctionTest, ConstParameters) {
Context = File;
- // A captured variable that's only read becomes a const reference.
+ // A captured scalar that's only read becomes a by-value parameter;
+ // non-scalars instead become a const reference (see the `S` cases
+ // below).
EXPECT_THAT(apply(R"cpp(
void use(int);
void f(int x) { [[use(x);]] }
)cpp"),
- HasSubstr("void extracted(const int &x)"));
+ HasSubstr("void extracted(int x)"));
// Direct assignment: stays non-const.
EXPECT_THAT(apply("void f(int x) { [[x = 1;]] }"),
HasSubstr("void extracted(int &x)"));
@@ -817,21 +819,29 @@ TEST_F(ExtractFunctionTest, ConstParameters) {
)cpp"),
HasSubstr("void extracted(int &x)"));
// Passed to a parameter taking a const reference or by value: becomes
- // const, since neither can mutate the caller's variable.
+ // an unmutated scalar, so by value.
EXPECT_THAT(apply(R"cpp(
void readOnly(const int &);
void f(int x) { [[readOnly(x);]] }
)cpp"),
- HasSubstr("void extracted(const int &x)"));
+ HasSubstr("void extracted(int x)"));
EXPECT_THAT(apply(R"cpp(
void byValue(int);
void f(int x) { [[byValue(x);]] }
)cpp"),
- HasSubstr("void extracted(const int &x)"));
- // A parameter that's already declared const stays as-is (no double
- // const).
+ HasSubstr("void extracted(int x)"));
+ // A scalar parameter that's already declared const drops that
+ // qualifier when passed by value: it'd be a no-op there.
EXPECT_THAT(apply("void use(int); void f(const int x) { [[use(x);]] }"),
- HasSubstr("void extracted(const int &x)"));
+ HasSubstr("void extracted(int x)"));
+ // A non-scalar parameter that's already declared const keeps that
+ // qualifier, since it's still passed by reference.
+ EXPECT_THAT(apply(R"cpp(
+ struct S {};
+ void use(const S &);
+ void f(const S s) { [[use(s);]] }
+ )cpp"),
+ HasSubstr("void extracted(const S &s)"));
}
TEST_F(ExtractFunctionTest, ConstParametersReferenceAliasing) {
@@ -864,9 +874,10 @@ TEST_F(ExtractFunctionTest, ConstParametersConservativeAliasing) {
// whether the lambda actually mutates it.
EXPECT_THAT(apply("void f(int x) { [[auto l = [&x]() { int y = x; };]] }"),
HasSubstr("int &x"));
- // Captured by value in a lambda: doesn't alias x, so becomes const.
+ // Captured by value in a lambda: doesn't alias x, so it's unmutated and
+ // (being a scalar) passed by value.
EXPECT_THAT(apply("void f(int x) { [[auto l = [x]() { int y = x; };]] }"),
- HasSubstr("const int &x"));
+ HasSubstr("void extracted(int x)"));
// Returning a captured variable is conservatively treated as a possible
// mutation, regardless of whether the return is actually by value (safe)
// or by non-const reference (not safe) -- telling these apart isn't
@@ -909,19 +920,20 @@ TEST_F(ExtractFunctionTest, ConstParametersMemberCallArguments) {
TEST_F(ExtractFunctionTest, ConstParametersPointerIndirection) {
Context = File;
// Mutating a member/element through a pointer only mutates the pointee,
- // never the pointer's own binding, so the pointer stays const-eligible --
+ // never the pointer's own binding, so the pointer itself is unmutated --
// unlike the same access through a value or reference (already covered
// by ConstParameters' `ptr` case, which is mutated directly instead).
+ // Being an unmutated scalar, it's then passed by value.
EXPECT_THAT(apply(R"cpp(
struct S { int x; };
void f(S *ptr) { [[ptr->x = 1;]] }
)cpp"),
- HasSubstr("extracted(S *const &ptr)"));
+ HasSubstr("extracted(S * ptr)"));
EXPECT_THAT(apply("void f(int *p) { [[p[0] = 1;]] }"),
- HasSubstr("extracted(int *const &p)"));
+ HasSubstr("extracted(int * p)"));
// Same for a plain dereference.
EXPECT_THAT(apply("void f(int *p) { [[*p = 1;]] }"),
- HasSubstr("extracted(int *const &p)"));
+ HasSubstr("extracted(int * p)"));
}
TEST_F(ExtractFunctionTest, ConstParametersConditionalReferenceBinding) {
@@ -936,7 +948,7 @@ TEST_F(ExtractFunctionTest, ConstParametersConditionalReferenceBinding) {
a = 5;]]
}
)cpp"),
- HasSubstr("extracted(const bool &cond, int &c, int &d)"));
+ HasSubstr("extracted(bool cond, int &c, int &d)"));
}
TEST_F(ExtractFunctionTest, ConstParametersConditionalMutatingAccess) {
@@ -948,7 +960,7 @@ TEST_F(ExtractFunctionTest, ConstParametersConditionalMutatingAccess) {
[[(cond ? s1 : s2).n = 0;]]
}
)cpp"),
- HasSubstr("extracted(const bool &cond, S &s1, S &s2)"));
+ HasSubstr("extracted(bool cond, S &s1, S &s2)"));
}
TEST_F(ExtractFunctionTest, ConstParametersStaticOperatorCall) {
@@ -961,7 +973,33 @@ TEST_F(ExtractFunctionTest, ConstParametersStaticOperatorCall) {
struct S { static void operator()(int); };
void f(S s, int x) { [[s(x);]] }
)cpp"),
- HasSubstr("extracted(const S &s, const int &x)"));
+ HasSubstr("extracted(const S &s, int x)"));
+}
+
+TEST_F(ExtractFunctionTest, ConstParametersScalarsByValue) {
+ Context = File;
+ // An unmutated pointer is a scalar too: passed by value.
+ EXPECT_THAT(apply("void use(int *); void f(int *p) { [[use(p);]] }"),
+ HasSubstr("extracted(int * p)"));
+ // An unmutated enum: passed by value.
+ EXPECT_THAT(apply(R"cpp(
+ enum E { A, B };
+ void use(E);
+ void f(E e) { [[use(e);]] }
+ )cpp"),
+ HasSubstr("extracted(E e)"));
+ // A class type, even one that's small and trivially copyable, is never
+ // passed by value: that's deliberately out of scope for now.
+ EXPECT_THAT(apply(R"cpp(
+ struct Point { int x, y; };
+ void use(Point);
+ void f(Point p) { [[use(p);]] }
+ )cpp"),
+ HasSubstr("extracted(const Point &p)"));
+ // An unmutated array is not a scalar (even though its element type is):
+ // stays a non-const reference, per the existing array carve-out.
+ EXPECT_THAT(apply("void f() { int arr[5]; [[int x = arr[0];]] }"),
+ HasSubstr("extracted(int[5] &arr)"));
}
} // namespace
>From d47a85ef3a6bb4b65d4112046fe6b5d94a261112 Mon Sep 17 00:00:00 2001
From: Christian Kandeler <christian.kandeler at qt.io>
Date: Thu, 1 Oct 2026 13:14:47 +0200
Subject: [PATCH 2/5] Address review: Do not pass variables of ref type by
value
---
.../refactor/tweaks/ExtractFunction.cpp | 5 ++--
.../unittests/tweaks/ExtractFunctionTests.cpp | 28 +++++++++++++++++++
2 files changed, 31 insertions(+), 2 deletions(-)
diff --git a/clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp b/clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp
index 3085f237fe10d..7dc7a3ca6b7b8 100644
--- a/clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp
+++ b/clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp
@@ -1013,7 +1013,8 @@ bool createParameters(NewFunction &ExtractedFunc,
if (!VD || isa<FunctionDecl>(DeclInfo.TheDecl))
return false;
// Parameter qualifiers are same as the Decl's qualifiers.
- QualType TypeInfo = VD->getType().getNonReferenceType();
+ QualType FullTypeInfo = VD->getType();
+ QualType TypeInfo = FullTypeInfo.getNonReferenceType();
// FIXME: check if parameter will be a non l-value reference.
bool IsPassedByReference = true;
if (!DeclInfo.IsPossiblyMutated) {
@@ -1021,7 +1022,7 @@ bool createParameters(NewFunction &ExtractedFunc,
// cheap to copy as to pass by reference, and less noisy. Any
// pre-existing const is dropped: it's a no-op on a by-value
// parameter, not a signal worth keeping.
- if (TypeInfo->isScalarType()) {
+ if (TypeInfo->isScalarType() && !FullTypeInfo->isReferenceType()) {
IsPassedByReference = false;
TypeInfo.removeLocalConst();
} else if (!TypeInfo->isArrayType()) {
diff --git a/clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp b/clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp
index 7648641c6ffc5..cea101d98cf80 100644
--- a/clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp
+++ b/clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp
@@ -1002,6 +1002,34 @@ TEST_F(ExtractFunctionTest, ConstParametersScalarsByValue) {
HasSubstr("extracted(int[5] &arr)"));
}
+// Variables of reference type, const or non-const, must stay references,
+// otherwise they'd stop tracking their target.
+TEST_F(ExtractFunctionTest, ReferenceToScalar) {
+ Context = File;
+ EXPECT_THAT(apply(R"cpp(
+ void bar(int) {}
+ void foo() {
+ int A = 0;
+ int &B = A;
+ [[
+ A = 1;
+ bar(B);
+ ]]
+ })cpp"),
+ HasSubstr("extracted(int &A, const int &B)"));
+ EXPECT_THAT(apply(R"cpp(
+ void bar(int) {}
+ void foo() {
+ int A = 0;
+ const int &B = A;
+ [[
+ A = 1;
+ bar(B);
+ ]]
+ })cpp"),
+ HasSubstr("extracted(int &A, const int &B)"));
+}
+
} // namespace
} // namespace clangd
} // namespace clang
>From fb483cf9400562521d8ab9b010a508d188c70ac9 Mon Sep 17 00:00:00 2001
From: Christian Kandeler <christian.kandeler at qt.io>
Date: Thu, 1 Oct 2026 13:33:52 +0200
Subject: [PATCH 3/5] Address review comment: Keep const qualifiers
... also when passing by value.
---
.../clangd/refactor/tweaks/ExtractFunction.cpp | 5 +----
.../clangd/unittests/tweaks/ExtractFunctionTests.cpp | 11 ++++++-----
2 files changed, 7 insertions(+), 9 deletions(-)
diff --git a/clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp b/clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp
index 7dc7a3ca6b7b8..b03b668d022ff 100644
--- a/clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp
+++ b/clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp
@@ -1019,12 +1019,9 @@ bool createParameters(NewFunction &ExtractedFunc,
bool IsPassedByReference = true;
if (!DeclInfo.IsPossiblyMutated) {
// A scalar (arithmetic, pointer, enumeration, ...) is at least as
- // cheap to copy as to pass by reference, and less noisy. Any
- // pre-existing const is dropped: it's a no-op on a by-value
- // parameter, not a signal worth keeping.
+ // cheap to copy as to pass by reference, and less noisy.
if (TypeInfo->isScalarType() && !FullTypeInfo->isReferenceType()) {
IsPassedByReference = false;
- TypeInfo.removeLocalConst();
} else if (!TypeInfo->isArrayType()) {
// Still passed by reference to avoid a copy, but the reference
// doesn't need to be mutable. Array types are never made const:
diff --git a/clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp b/clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp
index cea101d98cf80..ac268d4462cec 100644
--- a/clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp
+++ b/clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp
@@ -93,13 +93,13 @@ void f(int a) {
})cpp";
EXPECT_EQ(apply(ParameterCheckInput), ParameterCheckOutput);
- // Check const qualifier: dropped for a by-value scalar.
+ // Check const qualifier
std::string ConstCheckInput = R"cpp(
void f(const int c) {
[[while(c) {}]]
})cpp";
std::string ConstCheckOutput = R"cpp(
-void extracted(int c) {
+void extracted(const int c) {
while(c) {}
}
void f(const int c) {
@@ -830,10 +830,11 @@ TEST_F(ExtractFunctionTest, ConstParameters) {
void f(int x) { [[byValue(x);]] }
)cpp"),
HasSubstr("void extracted(int x)"));
- // A scalar parameter that's already declared const drops that
- // qualifier when passed by value: it'd be a no-op there.
+ // A scalar parameter that's already declared const keeps that
+ // qualifier when passed by value: It might be relevant for overload
+ // resolution.
EXPECT_THAT(apply("void use(int); void f(const int x) { [[use(x);]] }"),
- HasSubstr("void extracted(int x)"));
+ HasSubstr("void extracted(const int x)"));
// A non-scalar parameter that's already declared const keeps that
// qualifier, since it's still passed by reference.
EXPECT_THAT(apply(R"cpp(
>From 123bf3047cd74dc3139e4ae146ebbad50714c6f9 Mon Sep 17 00:00:00 2001
From: Christian Kandeler <christian.kandeler at qt.io>
Date: Thu, 1 Oct 2026 13:50:52 +0200
Subject: [PATCH 4/5] Address review comment: Guard against large scalar types
... when deciding whether to pass by value.
---
.../clangd/refactor/tweaks/ExtractFunction.cpp | 11 ++++++++---
.../clangd/unittests/tweaks/ExtractFunctionTests.cpp | 11 +++++++++++
2 files changed, 19 insertions(+), 3 deletions(-)
diff --git a/clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp b/clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp
index b03b668d022ff..b36dfb6bd39dd 100644
--- a/clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp
+++ b/clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp
@@ -992,7 +992,8 @@ CapturedZoneInfo captureZoneInfo(const ExtractionZone &ExtZone) {
// needed.
// FIXME: Check if the declaration has a local/anonymous type
bool createParameters(NewFunction &ExtractedFunc,
- const CapturedZoneInfo &CapturedInfo) {
+ const CapturedZoneInfo &CapturedInfo,
+ const ASTContext &Context) {
for (const auto &KeyVal : CapturedInfo.DeclInfoMap) {
const auto &DeclInfo = KeyVal.second;
// If a Decl was Declared in zone and referenced in post zone, it
@@ -1018,9 +1019,12 @@ bool createParameters(NewFunction &ExtractedFunc,
// FIXME: check if parameter will be a non l-value reference.
bool IsPassedByReference = true;
if (!DeclInfo.IsPossiblyMutated) {
+ auto WordSize = Context.getTypeSizeInChars(Context.VoidPtrTy);
+ auto TypeSize = Context.getTypeSizeInChars(TypeInfo);
// A scalar (arithmetic, pointer, enumeration, ...) is at least as
// cheap to copy as to pass by reference, and less noisy.
- if (TypeInfo->isScalarType() && !FullTypeInfo->isReferenceType()) {
+ if (TypeInfo->isScalarType() && !FullTypeInfo->isReferenceType() &&
+ TypeSize <= 2 * WordSize) {
IsPassedByReference = false;
} else if (!TypeInfo->isArrayType()) {
// Still passed by reference to avoid a copy, but the reference
@@ -1129,7 +1133,8 @@ llvm::Expected<NewFunction> getExtractedFunction(ExtractionZone &ExtZone,
ExtractedFunc.DefinitionPoint = ExtZone.getInsertionPoint();
ExtractedFunc.CallerReturnsValue = CapturedInfo.AlwaysReturns;
- if (!createParameters(ExtractedFunc, CapturedInfo) ||
+ if (!createParameters(ExtractedFunc, CapturedInfo,
+ ExtZone.EnclosingFunction->getASTContext()) ||
!generateReturnProperties(ExtractedFunc, *ExtZone.EnclosingFunction,
CapturedInfo))
return error("Too complex to extract.");
diff --git a/clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp b/clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp
index ac268d4462cec..0d30623c9f973 100644
--- a/clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp
+++ b/clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp
@@ -1031,6 +1031,17 @@ TEST_F(ExtractFunctionTest, ReferenceToScalar) {
HasSubstr("extracted(int &A, const int &B)"));
}
+TEST_F(ExtractFunctionTest, LargeScalarType) {
+ Context = File;
+ // _BitInt(256) is a scalar type, but far larger than a couple of
+ // words: stays by reference despite being unmutated.
+ EXPECT_THAT(apply(R"cpp(
+ void use(_BitInt(256));
+ void f(_BitInt(256) x) { [[use(x);]] }
+ )cpp"),
+ HasSubstr("extracted(const _BitInt(256) &x)"));
+}
+
} // namespace
} // namespace clangd
} // namespace clang
>From 9ea17acb15dee23e0f4817554f7ed725946f8a81 Mon Sep 17 00:00:00 2001
From: Christian Kandeler <christian.kandeler at qt.io>
Date: Thu, 1 Oct 2026 14:15:54 +0200
Subject: [PATCH 5/5] Address review comment: Do not pass volatiles by value
---
.../clangd/refactor/tweaks/ExtractFunction.cpp | 4 ++--
.../unittests/tweaks/ExtractFunctionTests.cpp | 13 +++++++++++++
2 files changed, 15 insertions(+), 2 deletions(-)
diff --git a/clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp b/clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp
index b36dfb6bd39dd..4ab3e5b26b2ec 100644
--- a/clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp
+++ b/clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp
@@ -1023,8 +1023,8 @@ bool createParameters(NewFunction &ExtractedFunc,
auto TypeSize = Context.getTypeSizeInChars(TypeInfo);
// A scalar (arithmetic, pointer, enumeration, ...) is at least as
// cheap to copy as to pass by reference, and less noisy.
- if (TypeInfo->isScalarType() && !FullTypeInfo->isReferenceType() &&
- TypeSize <= 2 * WordSize) {
+ if (TypeInfo->isScalarType() && !TypeInfo.isVolatileQualified() &&
+ !FullTypeInfo->isReferenceType() && TypeSize <= 2 * WordSize) {
IsPassedByReference = false;
} else if (!TypeInfo->isArrayType()) {
// Still passed by reference to avoid a copy, but the reference
diff --git a/clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp b/clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp
index 0d30623c9f973..7bdb6bd24fd6c 100644
--- a/clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp
+++ b/clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp
@@ -1042,6 +1042,19 @@ TEST_F(ExtractFunctionTest, LargeScalarType) {
HasSubstr("extracted(const _BitInt(256) &x)"));
}
+TEST_F(ExtractFunctionTest, VolatileScalar) {
+ Context = File;
+ EXPECT_THAT(apply(R"cpp(
+ void bar(const volatile int &, int) {}
+ void foo() {
+ volatile int V = 0;
+ [[
+ bar(V, 0);
+ ]]
+ })cpp"),
+ HasSubstr("extracted(const volatile int &V)"));
+}
+
} // namespace
} // namespace clangd
} // namespace clang
More information about the cfe-commits
mailing list