[clang-tools-extra] 186e70f - [clangd] Extract to function: Suppress pure forwarding wrappers (#232654)
via cfe-commits
cfe-commits at lists.llvm.org
Wed Oct 21 03:50:16 PDT 2026
Author: Christian Kandeler
Date: 2026-10-21T12:50:11+02:00
New Revision: 186e70f4e6a1521991d2340d3d49b23b1beff11f
URL: https://github.com/llvm/llvm-project/commit/186e70f4e6a1521991d2340d3d49b23b1beff11f
DIFF: https://github.com/llvm/llvm-project/commit/186e70f4e6a1521991d2340d3d49b23b1beff11f.diff
LOG: [clangd] Extract to function: Suppress pure forwarding wrappers (#232654)
Extracting a single statement that's just a lone call (or a return of
one), where every operand that would need to become a parameter is
already exactly that parameter value unchanged, produces nothing but a
renamed call: `foo(x);` becomes `void extracted(int x) { foo(x); }`
called as `extracted(x);`, with no abstraction gained. A value-less
`return;` is the same kind of no-op: there's nothing to abstract over
at all. Both are now rejected.
This is deliberately narrow and per-operand, not a naive argument-count
check: `foo(100)` (a literal), `foo(globalVar)` (an already-visible
global), and `foo(x + y)` (real computation) are all still offered,
since each does something a plain rename wouldn't.
Assisted-by: Claude Code
Added:
Modified:
clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp
clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp
Removed:
################################################################################
diff --git a/clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp b/clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp
index de9a5fa45b9c0..7b60b95bd61f2 100644
--- a/clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp
+++ b/clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp
@@ -390,13 +390,152 @@ computeEnclosingFuncRange(const FunctionDecl *EnclosingFunction,
return toHalfOpenFileRange(SM, LangOpts, EnclosingFunction->getSourceRange());
}
+// Whether D would need to become a forwarded parameter of the extracted
+// function if referenced from inside the extraction zone: a name (a
+// parameter, local variable, or structured-binding) declared inside the
+// enclosing function. Such a name is only visible within that function,
+// regardless of storage duration -- unlike hasLocalStorage(), this is
+// also true of a function-scope `static` or `thread_local` variable,
+// which (unlike a namespace-scope global or a static data member, both
+// reachable via a qualified name from anywhere) isn't visible wherever
+// the extracted function ends up.
+bool mustBeForwarded(const Decl *D) {
+ if (!isa<VarDecl, BindingDecl>(D))
+ return false;
+ return D->getDeclContext()->isFunctionOrMethod();
+}
+
+// Whether E, as an operand of a call being considered by isTrivialExtraction,
+// is a bare (parenthesized/implicitly cast) reference to a Decl that would
+// need to become a parameter, forwarded unchanged -- and if so, that Decl.
+// Returns null otherwise: a literal, a reference to an already-visible
+// global, or any non-bare expression all mean this operand alone justifies
+// offering the extraction, since it hardcodes or computes something a
+// plain rename wouldn't.
+const Decl *bareForwardedDecl(const Expr *E) {
+ const auto *DRE = dyn_cast<DeclRefExpr>(E->IgnoreParenImpCasts());
+ if (!DRE || !mustBeForwarded(DRE->getDecl()))
+ return nullptr;
+ return DRE->getDecl();
+}
+
+// The operands of Call relevant to isTrivialExtraction: the receiver of
+// a method call, a non-method callee that isn't a plain reference to a
+// fixed function (e.g. a function-pointer variable instead), and every
+// explicit argument. An operator call's operands are already included in
+// its own arguments() (clang models e.g. `a + b` as `operator+(a, b)`),
+// so its callee -- always a reference to the operator function/overload
+// itself, never itself customizable -- is always excluded.
+llvm::SmallVector<const Expr *, 4> relevantCallOperands(const CallExpr *Call) {
+ llvm::SmallVector<const Expr *, 4> Operands;
+ if (const auto *MCE = dyn_cast<CXXMemberCallExpr>(Call)) {
+ // getImplicitObjectArgument() can itself be null (its FIXME notes
+ // this isn't fully handled for all cases); other clang analyses
+ // (e.g. LiveVariables, dataflow's Environment::pushCall) guard it
+ // the same way rather than assume it's always present.
+ if (const Expr *ImplicitObject = MCE->getImplicitObjectArgument()) {
+ // `this` (implicit or explicit) always refers to the same object
+ // -- the extracted function becomes a sibling member itself, so
+ // `this` stays implicit there too, never becoming a parameter --
+ // so, like an ordinary function name, it's excluded rather than
+ // required to be "bare".
+ if (!isa<CXXThisExpr>(ImplicitObject->IgnoreParenImpCasts()))
+ Operands.push_back(ImplicitObject);
+ }
+ // A pointer-to-member call (`(s.*pmf)(x)`) has no statically known
+ // method to exclude the way an ordinary method call's name is
+ // excluded below: which member is invoked is itself a real,
+ // customizable operand here (e.g. the `pmf` it's called through),
+ // not inherent to what the wrapper does.
+ if (!MCE->getMethodDecl())
+ Operands.push_back(MCE->getCallee());
+ } else if (!isa<CXXOperatorCallExpr>(Call)) {
+ const Expr *Callee = Call->getCallee()->IgnoreParenImpCasts();
+ if (!(isa<DeclRefExpr>(Callee) &&
+ isa<FunctionDecl>(cast<DeclRefExpr>(Callee)->getDecl())))
+ Operands.push_back(Call->getCallee());
+ }
+ // An omitted argument filled in from the callee's own default isn't
+ // something this call site wrote or hardcoded: it's inherent to
+ // calling the function at all, same as the function's name, so it's
+ // excluded rather than required to be "bare". Default arguments are
+ // always trailing, so the first one found marks the end of the
+ // explicitly-written ones.
+ auto Args = Call->arguments();
+ auto FirstDefault = llvm::find_if(
+ Args, [](const Expr *Arg) { return isa<CXXDefaultArgExpr>(Arg); });
+ Operands.append(Args.begin(), FirstDefault);
+ return Operands;
+}
+
+// Whether extracting S on its own, as a single root statement, would add
+// no real abstraction over leaving it where it is:
+// - A value-less `return;` has nothing to abstract over at all.
+// - A call (to a function, method, or operator), or a return of one, is
+// a pure 1:1 forwarding wrapper if every relevant operand (see
+// relevantCallOperands) is already exactly the parameter value it
+// would become, unchanged (or there are none). Extracting e.g.
+// `foo();`, `foo(x)` (x a local), or `return foo(x);` this way would
+// just rename the call; `foo(100)`, `foo(globalVar)`, or `foo(x + y)`
+// would not, since each does something a plain rename wouldn't
+// (hardcode a value/global, or compute one). For the return case, the
+// returned value may be parenthesized and/or need an implicit
+// conversion to the enclosing function's own return type (e.g. foo
+// returns int, the enclosing function long); IgnoreParenImpCasts()
+// looks through both -- neither changes the verdict, since the
+// extracted function would perform the exact same conversion
+// internally.
+// - Using the same captured Decl in more than one operand (e.g.
+// `foo(A, A)`) is, despite every individual operand being bare,
+// *not* pure forwarding: it constrains a relationship (both slots
+// must hold the same value) that a plain rename wouldn't, the same
+// kind of real specialization as hardcoding a literal.
+// - createParameters orders the extracted function's own parameters by
+// each captured Decl's declaration order in the *enclosing* function,
+// not by how they're used in this call. So if a later operand here
+// refers to a Decl declared *earlier* than an operand before it (e.g.
+// `foo(B, A)` inside `bar(int A, int B)`), the parameters end up
+// reordered relative to this call: `extracted(int A, int B) { foo(B,
+// A); }`. That's not a transparent pass-through either, even though
+// this one call site still behaves the same after the compensating
+// reorder at the call site (`extracted(A, B)`) -- `extracted`'s own
+// signature no longer predicts its behavior the way a plain rename's
+// would, since it silently permutes its own parameters internally.
+bool isTrivialExtraction(const Stmt *S, const SourceManager &SM) {
+ if (const auto *RS = dyn_cast<ReturnStmt>(S)) {
+ if (!RS->getRetValue())
+ return true;
+ S = RS->getRetValue()->IgnoreParenImpCasts();
+ }
+ const auto *Call = dyn_cast<CallExpr>(S);
+ if (!Call)
+ return false;
+ llvm::SmallPtrSet<const Decl *, 4> SeenDecls;
+ SourceLocation LastDeclLoc;
+ for (const Expr *Operand : relevantCallOperands(Call)) {
+ const Decl *D = bareForwardedDecl(Operand);
+ if (!D || !SeenDecls.insert(D).second)
+ return false;
+ SourceLocation DeclLoc = D->getLocation();
+ if (LastDeclLoc.isValid() &&
+ !SM.isBeforeInTranslationUnit(LastDeclLoc, DeclLoc))
+ return false;
+ LastDeclLoc = DeclLoc;
+ }
+ return true;
+}
+
// returns true if Child can be a single RootStmt being extracted from
// EnclosingFunc.
-bool validSingleChild(const Node *Child, const FunctionDecl *EnclosingFunc) {
+bool validSingleChild(const Node *Child, const FunctionDecl *EnclosingFunc,
+ const SourceManager &SM) {
// Extracting the body of EnclosingFunc would remove it's definition.
assert(EnclosingFunc->hasBody() &&
"We should always be extracting from a function body.");
- if (Child->ASTNode.get<Stmt>() == EnclosingFunc->getBody())
+ const Stmt *S = Child->ASTNode.get<Stmt>();
+ if (S == EnclosingFunc->getBody())
+ return false;
+ if (isTrivialExtraction(S, SM))
return false;
return true;
}
@@ -414,7 +553,8 @@ std::optional<ExtractionZone> findExtractionZone(const Node *CommonAnc,
// When there is a single RootStmt, we must check if it's valid for
// extraction.
if (ExtZone.Parent->Children.size() == 1 &&
- !validSingleChild(ExtZone.getLastRootStmt(), ExtZone.EnclosingFunction))
+ !validSingleChild(ExtZone.getLastRootStmt(), ExtZone.EnclosingFunction,
+ SM))
return std::nullopt;
if (auto FuncRange =
computeEnclosingFuncRange(ExtZone.EnclosingFunction, SM, LangOpts))
diff --git a/clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp b/clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp
index fd2472655af37..981373b893347 100644
--- a/clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp
+++ b/clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp
@@ -744,7 +744,7 @@ TEST_F(ExtractFunctionTest, RangeBasedFor) {
struct Vec { int *begin(); int *end(); };
Vec V;
void foo(int);
- void f() { for (auto X : V) { [[foo(X);]] } }
+ void f() { for (auto X : V) { [[foo(X + 1);]] } }
)cpp"),
HasSubstr("extracted"));
}
@@ -782,10 +782,11 @@ TEST_F(ExtractFunctionTest, ConstParameters) {
Context = File;
// A captured scalar that's only read becomes a by-value parameter;
// non-scalars instead become a const reference (see the `S` cases
- // below).
+ // below). The duplicated calls below are there to prevent "pure forwarding"
+ // suppression.
EXPECT_THAT(apply(R"cpp(
void use(int);
- void f(int x) { [[use(x);]] }
+ void f(int x) { [[use(x); use(x);]] }
)cpp"),
HasSubstr("void extracted(int x)"));
// Direct assignment: stays non-const.
@@ -800,45 +801,46 @@ TEST_F(ExtractFunctionTest, ConstParameters) {
// A non-const method call may mutate the object: stays non-const.
EXPECT_THAT(apply(R"cpp(
struct S { void mutate(); };
- void f(S s) { [[s.mutate();]] }
+ void f(S s) { [[s.mutate(); s.mutate();]] }
)cpp"),
HasSubstr("void extracted(S &s)"));
// A const method call cannot mutate the object: becomes const.
EXPECT_THAT(apply(R"cpp(
struct S { void inspect() const; };
- void f(S s) { [[s.inspect();]] }
+ void f(S s) { [[s.inspect(); s.inspect();]] }
)cpp"),
HasSubstr("void extracted(const S &s)"));
// Passed to a parameter taking a non-const reference: stays non-const,
// since the callee could mutate it through that reference.
EXPECT_THAT(apply(R"cpp(
void mayMutate(int &);
- void f(int x) { [[mayMutate(x);]] }
+ void f(int x) { [[mayMutate(x); mayMutate(x);]] }
)cpp"),
HasSubstr("void extracted(int &x)"));
// Passed to a parameter taking a const reference or by value: becomes
// an unmutated scalar, so by value.
EXPECT_THAT(apply(R"cpp(
void readOnly(const int &);
- void f(int x) { [[readOnly(x);]] }
+ void f(int x) { [[readOnly(x); readOnly(x);]] }
)cpp"),
HasSubstr("void extracted(int x)"));
EXPECT_THAT(apply(R"cpp(
void byValue(int);
- void f(int x) { [[byValue(x);]] }
+ void f(int x) { [[byValue(x); byValue(x);]] }
)cpp"),
HasSubstr("void extracted(int x)"));
// 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(const int x)"));
+ EXPECT_THAT(
+ apply("void use(int); void f(const int x) { [[use(x); use(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(
struct S {};
void use(const S &);
- void f(const S s) { [[use(s);]] }
+ void f(const S s) { [[use(s); use(s);]] }
)cpp"),
HasSubstr("void extracted(const S &s)"));
}
@@ -895,7 +897,7 @@ TEST_F(ExtractFunctionTest, ConstParametersMemberCallArguments) {
// the argument check from the object check.
EXPECT_THAT(apply(R"cpp(
struct S { void mayMutate(int &) const; };
- void f(S s, int x) { [[s.mayMutate(x);]] }
+ void f(S s, int x) { [[s.mayMutate(x); s.mayMutate(x);]] }
)cpp"),
HasSubstr("extracted(const S &s, int &x)"));
// Same, but through an overloaded operator: the implicit object is
@@ -903,7 +905,7 @@ TEST_F(ExtractFunctionTest, ConstParametersMemberCallArguments) {
// found at the right offset.
EXPECT_THAT(apply(R"cpp(
struct S { void operator()(int &) const; };
- void f(S s, int x) { [[s(x);]] }
+ void f(S s, int x) { [[s(x); s(x);]] }
)cpp"),
HasSubstr("extracted(const S &s, int &x)"));
// A free (non-member) operator overload has no implicit object, so all
@@ -911,7 +913,7 @@ TEST_F(ExtractFunctionTest, ConstParametersMemberCallArguments) {
EXPECT_THAT(apply(R"cpp(
struct S {};
void operator+(const S &, int &);
- void f(S s, int x) { [[s + x;]] }
+ void f(S s, int x) { [[s + x; s + x;]] }
)cpp"),
HasSubstr("extracted(const S &s, int &x)"));
}
@@ -970,7 +972,7 @@ TEST_F(ExtractFunctionTest, ConstParametersStaticOperatorCall) {
ExtraArgs.push_back("-std=c++23");
EXPECT_THAT(apply(R"cpp(
struct S { static void operator()(int); };
- void f(S s, int x) { [[s(x);]] }
+ void f(S s, int x) { [[s(x); s(x);]] }
)cpp"),
HasSubstr("extracted(const S &s, int x)"));
}
@@ -998,13 +1000,13 @@ TEST_F(ExtractFunctionTest, ConstParametersFunctionReference) {
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);]] }"),
+ EXPECT_THAT(apply("void use(int *); void f(int *p) { [[use(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);]] }
+ void f(E e) { [[use(e); use(e);]] }
)cpp"),
HasSubstr("extracted(E e)"));
// A class type, even one that's small and trivially copyable, is never
@@ -1332,6 +1334,290 @@ TEST_F(ExtractFunctionTest, CFileRejectMutatedPointerToVariableLengthArray) {
"fail: Too complex to extract.");
}
+TEST_F(ExtractFunctionTest, RejectPureForwardingZeroArgCall) {
+ Context = File;
+ // No abstraction gained: the extracted function would just rename the
+ // call, with no customizable behavior at all.
+ EXPECT_EQ(apply("void foo(); void f() { [[foo();]] }"), "unavailable");
+}
+
+TEST_F(ExtractFunctionTest, RejectPureForwardingSingleArgCall) {
+ Context = File;
+ // x is forwarded to foo unchanged: still just a rename.
+ EXPECT_EQ(apply("void foo(int); void f(int x) { [[foo(x);]] }"),
+ "unavailable");
+}
+
+TEST_F(ExtractFunctionTest, OfferDespitePureForwarding_LiteralArg) {
+ Context = File;
+ // Hardcoding 100 is a real specialization, unlike a plain rename.
+ EXPECT_THAT(apply("void foo(int); void f() { [[foo(100);]] }"),
+ HasSubstr("extracted"));
+}
+
+TEST_F(ExtractFunctionTest, OfferDespitePureForwarding_GlobalArg) {
+ Context = File;
+ // Hardcoding which already-visible global to use is likewise a real
+ // specialization: unlike a captured local, globalVar doesn't need to
+ // become a parameter at all.
+ EXPECT_THAT(apply(R"cpp(
+ int globalVar;
+ void foo(int);
+ void f() { [[foo(globalVar);]] }
+ )cpp"),
+ HasSubstr("extracted"));
+}
+
+TEST_F(ExtractFunctionTest, OfferDespitePureForwarding_ComputedArg) {
+ Context = File;
+ // foo(x + y) needs 2 parameters for 1 argument slot: real computation,
+ // not just forwarding x and y unchanged.
+ EXPECT_THAT(apply("void foo(int); void f(int x, int y) { [[foo(x + y);]] }"),
+ HasSubstr("extracted"));
+}
+
+TEST_F(ExtractFunctionTest, RejectPureForwardingMethodCall) {
+ Context = File;
+ // Both the receiver and the argument are forwarded unchanged.
+ EXPECT_EQ(apply(R"cpp(
+ struct S { void method(int); };
+ void f(S s, int x) { [[s.method(x);]] }
+ )cpp"),
+ "unavailable");
+}
+
+TEST_F(ExtractFunctionTest, OfferDespitePureForwarding_GlobalReceiver) {
+ Context = File;
+ // Hardcoding which already-visible object to call the method on is a
+ // real specialization, same as a global argument.
+ EXPECT_THAT(apply(R"cpp(
+ struct S { void method(); };
+ S globalS;
+ void f() { [[globalS.method();]] }
+ )cpp"),
+ HasSubstr("extracted"));
+}
+
+TEST_F(ExtractFunctionTest, RejectPureForwardingOperatorCall) {
+ Context = File;
+ // Both operands of a + b are forwarded unchanged; the operator function
+ // itself (never customizable) is correctly excluded from the check.
+ EXPECT_EQ(apply(R"cpp(
+ struct S {};
+ void operator+(const S &, int);
+ void f(S s, int x) { [[s + x;]] }
+ )cpp"),
+ "unavailable");
+}
+
+TEST_F(ExtractFunctionTest, RejectPureForwardingFunctionPointerCall) {
+ Context = File;
+ // Unlike an ordinary function name, a function-pointer variable callee
+ // does need to become a parameter -- but it's still forwarded
+ // unchanged here, same as x.
+ EXPECT_EQ(apply("void f(void (*fp)(int), int x) { [[fp(x);]] }"),
+ "unavailable");
+}
+
+TEST_F(ExtractFunctionTest, OfferDespitePureForwarding_ComputedCallee) {
+ Context = File;
+ // Which function pointer gets called is itself computed here, not a
+ // bare forwarded reference: real logic, not just forwarding.
+ EXPECT_THAT(
+ apply("void f(void (*fp[2])(int), int i, int x) { [[fp[i](x);]] }"),
+ HasSubstr("extracted"));
+}
+
+TEST_F(ExtractFunctionTest, RejectPureForwardingReturnStmt) {
+ Context = File;
+ // A call wrapped in a ReturnStmt is just as much a pure rename as a
+ // bare call statement.
+ EXPECT_EQ(apply("int foo(int); int f(int x) { [[return foo(x);]] }"),
+ "unavailable");
+}
+
+TEST_F(ExtractFunctionTest, RejectPureForwardingReturnStmtImplicitConversion) {
+ Context = File;
+ // The implicit int-to-long conversion doesn't change the verdict: the
+ // extracted function would perform the exact same conversion
+ // internally, so this is still just a rename.
+ EXPECT_EQ(apply("int foo(int); long f(int x) { [[return foo(x);]] }"),
+ "unavailable");
+}
+
+TEST_F(ExtractFunctionTest, OfferDespitePureForwarding_ReturnStmtComputedArg) {
+ Context = File;
+ // Same as the non-return case: a computed argument is real abstraction.
+ EXPECT_THAT(
+ apply("int foo(int); int f(int x, int y) { [[return foo(x + y);]] }"),
+ HasSubstr("extracted"));
+}
+
+TEST_F(ExtractFunctionTest, RejectBareReturnStmt) {
+ Context = File;
+ // A value-less return has nothing to abstract over at all: extracting
+ // it would just wrap it in a function that does the exact same thing.
+ EXPECT_EQ(apply("void f() { [[return;]] }"), "unavailable");
+}
+
+TEST_F(ExtractFunctionTest, OfferDespitePureForwarding_PointerToMemberCallee) {
+ Context = File;
+ // Unlike an ordinary method call, a pointer-to-member call has no
+ // statically known method (getMethodDecl() is null): which member is
+ // invoked is itself computed here (table[I]), not inherent to what the
+ // wrapper does, so it must be checked like any other operand.
+ EXPECT_THAT(apply(R"cpp(
+ struct St {};
+ void f(St S, void (St::*table[2])(int), int I, int A) {
+ [[(S.*table[I])(A);]]
+ })cpp"),
+ HasSubstr("extracted"));
+}
+
+TEST_F(ExtractFunctionTest, RejectPureForwardingImplicitThis) {
+ Context = File;
+ // An implicit `this` receiver always refers to the same object -- the
+ // extracted function becomes a sibling member itself, so `this` stays
+ // implicit there too -- same as an ordinary function name, it must not
+ // disqualify suppression just for not being a "bare" reference.
+ EXPECT_EQ(apply(R"cpp(
+ struct S {
+ void foo(int);
+ void bar(int A) {
+ [[foo(A);]]
+ }
+ };)cpp"),
+ "unavailable");
+}
+
+TEST_F(ExtractFunctionTest, RejectPureForwardingDefaultArg) {
+ Context = File;
+ // An argument filled in from the callee's own default (as opposed to
+ // one actually written at this call site) isn't something to hardcode
+ // a judgement about: it must not disqualify suppression.
+ EXPECT_EQ(apply(R"cpp(
+ void foo(int A, int B = 0);
+ void bar(int A) {
+ [[foo(A);]]
+ })cpp"),
+ "unavailable");
+}
+
+TEST_F(ExtractFunctionTest, RejectPureForwardingParenthesizedReturn) {
+ Context = File;
+ // Parentheses around the returned call must be looked through, same as
+ // an implicit conversion.
+ EXPECT_EQ(apply(R"cpp(
+ int foo(int);
+ int bar(int A) {
+ [[return (foo(A));]]
+ })cpp"),
+ "unavailable");
+}
+
+TEST_F(ExtractFunctionTest, OfferDespitePureForwarding_RepeatedArg) {
+ Context = File;
+ // Using the same captured local for more than one argument constrains a
+ // relationship (both slots must hold the same value) that a plain
+ // rename wouldn't: the same kind of real specialization as hardcoding
+ // a literal, even though each individual operand is bare.
+ EXPECT_THAT(apply(R"cpp(
+ void foo(int A, int B);
+ void bar(int A, int B) {
+ [[foo(A, A);]]
+ })cpp"),
+ HasSubstr("extracted"));
+}
+
+TEST_F(ExtractFunctionTest, OfferDespitePureForwarding_ReorderedArgs) {
+ Context = File;
+ // createParameters orders the extracted function's parameters by each
+ // Decl's declaration order in bar (A, then B), not by how they're used
+ // here -- so despite every argument being an independently bare,
+ // distinct local, the extracted function ends up permuting its own
+ // parameters internally (extracted(int A, int B) { foo(B, A); }): not
+ // a transparent pass-through, since its signature alone no longer
+ // predicts its behavior the way a plain rename's would.
+ EXPECT_THAT(apply(R"cpp(
+ void foo(int A, int B);
+ void bar(int A, int B) {
+ [[foo(B, A);]]
+ })cpp"),
+ HasSubstr("extracted"));
+}
+
+TEST_F(ExtractFunctionTest, RejectPureForwardingPreservedArgOrder) {
+ Context = File;
+ // Same distinct locals as the reordered case above, but passed in the
+ // same order they're declared in bar: no permutation is hidden in the
+ // extracted function, so this is still just a rename.
+ EXPECT_EQ(apply(R"cpp(
+ void foo(int A, int B);
+ void bar(int A, int B) {
+ [[foo(A, B);]]
+ })cpp"),
+ "unavailable");
+}
+
+TEST_F(ExtractFunctionTest, RejectPureForwardingFunctionScopeStatic) {
+ Context = File;
+ // A function-scope static variable's name, unlike a namespace-scope
+ // global's, isn't visible wherever the extracted function ends up: it
+ // does need forwarding, same as an ordinary local variable, so this is
+ // still just a rename.
+ EXPECT_EQ(apply(R"cpp(
+ void foo(int);
+ void bar() {
+ static int A;
+ [[foo(A);]]
+ })cpp"),
+ "unavailable");
+}
+
+TEST_F(ExtractFunctionTest, RejectPureForwardingFunctionScopeThreadLocal) {
+ Context = File;
+ // Same as the static case: thread_local doesn't change the variable's
+ // name visibility, only its storage.
+ EXPECT_EQ(apply(R"cpp(
+ void foo(int);
+ void bar() {
+ thread_local int A;
+ [[foo(A);]]
+ })cpp"),
+ "unavailable");
+}
+
+TEST_F(ExtractFunctionTest, OfferDespitePureForwarding_NamespaceScopeStatic) {
+ Context = File;
+ // Unlike a function-scope static, a namespace-scope one (internal
+ // linkage notwithstanding) is visible wherever the extracted function
+ // ends up, same as an ordinary global: hardcoding it is a real
+ // specialization.
+ EXPECT_THAT(apply(R"cpp(
+ static int A;
+ void foo(int);
+ void bar() { [[foo(A);]] }
+ )cpp"),
+ HasSubstr("extracted"));
+}
+
+TEST_F(ExtractFunctionTest, RejectPureForwardingStructuredBinding) {
+ Context = File;
+ // A structured-binding name is declared by a BindingDecl, not a
+ // VarDecl, but is just as function-local and needs forwarding the
+ // same way.
+ ExtraArgs.push_back("-std=c++17");
+ EXPECT_EQ(apply(R"cpp(
+ void foo(int);
+ struct Pair { int A, B; };
+ Pair make();
+ void bar() {
+ auto [A, B] = make();
+ [[foo(A);]]
+ })cpp"),
+ "unavailable");
+}
+
} // namespace
} // namespace clangd
} // namespace clang
More information about the cfe-commits
mailing list