[clang-tools-extra] [clangd] Extract to function: Do not reject unconditionally in C files (PR #228465)
Christian Kandeler via cfe-commits
cfe-commits at lists.llvm.org
Wed Oct 7 03:06:22 PDT 2026
https://github.com/ckandeler updated https://github.com/llvm/llvm-project/pull/228465
>From 1f31530ab5f3f95b5d81e43ec73feda51aa5026a Mon Sep 17 00:00:00 2001
From: Christian Kandeler <christian.kandeler at qt.io>
Date: Fri, 2 Oct 2026 14:29:53 +0200
Subject: [PATCH 1/4] [clangd] Extract to function: Do not reject
unconditionally in C files
If the parameters can be passed by value, extraction in C files works
the same way as for C++. Otherwise, due to the lack of references, a
pointer parameter is used instead, with the call site taking its address
and every use inside the extracted body rewritten into a dereference.
Assisted-by: Claude Code
Closes https://github.com/clangd/clangd/issues/1810
---
.../refactor/tweaks/ExtractFunction.cpp | 155 ++++++++++++++++--
.../unittests/tweaks/ExtractFunctionTests.cpp | 80 ++++++++-
2 files changed, 219 insertions(+), 16 deletions(-)
diff --git a/clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp b/clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp
index 33a92daa07ccd..d98ba07e7a49e 100644
--- a/clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp
+++ b/clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp
@@ -28,6 +28,10 @@
// code become const references, except scalars (arithmetic, pointer,
// enumeration, ...), which are passed by value instead.
// - Otherwise passed by non-const reference
+// - In C, which has no references, a parameter that would otherwise need
+// one becomes a real pointer instead (except array types, which decay to
+// a pointer on their own): the call site takes its address, and every
+// use inside the extracted body is rewritten into a dereference.
// - Void return type
// - Cannot extract declarations that will be needed in the original function
// after extraction.
@@ -97,6 +101,17 @@ enum FunctionDeclKind {
OutOfLineDefinition
};
+// How a captured variable is passed to the extracted function.
+enum class ParamPassKind {
+ Value, // A plain copy: `T name`. Also used for a C array, which
+ // decays to a pointer on its own wherever it's used.
+ Reference, // C++ only: `T &name`.
+ Pointer, // C only (no references there): a real pointer, `T *name`,
+ // with the call site taking the address of the original
+ // variable and every use inside the extracted body rewritten
+ // into a dereference (see createParameters and getFuncBody).
+};
+
// Whether N, despite being Unselected, may still be a single RootStmt: a
// DeclStmt can be unselected since VarDecls claim the entire selection range
// in the selection tree. Similarly, a CXXOperatorCallExpr of a binary
@@ -421,7 +436,7 @@ struct NewFunction {
struct Parameter {
std::string Name;
QualType TypeInfo;
- bool PassByReference;
+ ParamPassKind Kind;
unsigned OrderPriority; // Lower value parameters are preferred first.
std::string render(const DeclContext *Context) const;
bool operator<(const Parameter &Other) const {
@@ -444,6 +459,23 @@ struct NewFunction {
ConstexprSpecKind Constexpr = ConstexprSpecKind::Unspecified;
bool Const = false;
+ // For C only: describes how to rewrite a single in-body reference to a
+ // parameter that had to become a real pointer instead of a reference (C
+ // has none), because its corresponding Parameter::Kind is Pointer. See
+ // createParameters and getFuncBody.
+ struct PointerRewriteSite {
+ SourceLocation Loc; // Location of the identifier itself.
+ unsigned NameLength;
+ // Set when this occurrence is the base of a non-arrow member access
+ // (`name.member`) immediately following it: that reads more
+ // naturally rewritten as `name->member` than `(*name).member`. Holds
+ // the location of the '.' token to replace with "->"; the identifier
+ // itself is then left untouched. Unset otherwise, in which case the
+ // identifier is instead wrapped in "(*...)".
+ std::optional<SourceLocation> DotLoc;
+ };
+ std::vector<PointerRewriteSite> PointerRewriteSites;
+
// Decides whether the extracted function body and the function call need a
// semicolon after extraction.
tooling::ExtractionSemicolonPolicy SemicolonPolicy;
@@ -490,6 +522,8 @@ std::string NewFunction::renderParametersForCall() const {
if (NeedCommaBefore)
Result += ", ";
NeedCommaBefore = true;
+ if (P.Kind == ParamPassKind::Pointer)
+ Result += "&";
Result += P.Name;
}
return Result;
@@ -570,12 +604,45 @@ std::string NewFunction::getFuncBody(const SourceManager &SM) const {
// - hoist decls
// - add return statement
// - Add semicolon
- return toSourceCode(SM, BodyRange).str() +
- (SemicolonPolicy.isNeededInExtractedFunction() ? ";" : "");
+ std::string Body;
+ if (PointerRewriteSites.empty()) {
+ Body = toSourceCode(SM, BodyRange).str();
+ } else {
+ // Splice in each site's rewrite, keeping everything else verbatim.
+ auto Sites = PointerRewriteSites;
+ llvm::sort(Sites, [&SM](const auto &A, const auto &B) {
+ return SM.isBeforeInTranslationUnit(A.Loc, B.Loc);
+ });
+ FileID FID = SM.getFileID(BodyRange.getBegin());
+ StringRef Buf = SM.getBufferOrFake(FID).getBuffer();
+ unsigned Cursor = SM.getFileOffset(BodyRange.getBegin());
+ for (const auto &Site : Sites) {
+ unsigned SiteBegin = SM.getFileOffset(Site.Loc);
+ Body += Buf.substr(Cursor, SiteBegin - Cursor);
+ if (Site.DotLoc) {
+ // Leave the identifier itself untouched; turn the member access
+ // that follows it into "->" instead of wrapping in a dereference.
+ Body += Buf.substr(SiteBegin, Site.NameLength);
+ Cursor = SiteBegin + Site.NameLength;
+ unsigned DotOffset = SM.getFileOffset(*Site.DotLoc);
+ Body += Buf.substr(Cursor, DotOffset - Cursor);
+ Body += "->";
+ Cursor = DotOffset + 1;
+ } else {
+ Body += "(*";
+ Body += Buf.substr(SiteBegin, Site.NameLength);
+ Body += ")";
+ Cursor = SiteBegin + Site.NameLength;
+ }
+ }
+ Body += Buf.substr(Cursor, SM.getFileOffset(BodyRange.getEnd()) - Cursor);
+ }
+ return Body + (SemicolonPolicy.isNeededInExtractedFunction() ? ";" : "");
}
std::string NewFunction::Parameter::render(const DeclContext *Context) const {
- return printType(TypeInfo, *Context) + (PassByReference ? " &" : " ") + Name;
+ return printType(TypeInfo, *Context) +
+ (Kind == ParamPassKind::Reference ? " &" : " ") + Name;
}
// Stores captured information about Extraction Zone.
@@ -591,6 +658,22 @@ struct CapturedZoneInfo {
// ExtractionZoneVisitor::markPossiblyMutated() for what "conservatively"
// means here.
bool IsPossiblyMutated = false;
+ // Describes one reference to this Decl inside the zone (not
+ // before/after it). Used to rewrite each use if this ends up becoming
+ // a C pointer-adapter parameter (see createParameters): C has no
+ // references, so a parameter that needs reference semantics there
+ // becomes a real pointer instead, and every use of it in the
+ // copied-out body must be adjusted accordingly.
+ struct Occurrence {
+ SourceLocation Loc; // Location of the identifier itself.
+ // Set when this occurrence is the base of a non-arrow member access
+ // (`name.member`) immediately following it: holds the location of
+ // the '.' token, so it can be turned into "->" instead of wrapping
+ // the identifier in a dereference. See
+ // NewFunction::PointerRewriteSite.
+ std::optional<SourceLocation> DotLoc;
+ };
+ llvm::SmallVector<Occurrence, 1> ZoneOccurrences;
DeclInformation(const Decl *TheDecl, ZoneRelative DeclaredIn,
unsigned DeclIndex)
: TheDecl(TheDecl), DeclaredIn(DeclaredIn), DeclIndex(DeclIndex){};
@@ -756,6 +839,21 @@ CapturedZoneInfo captureZoneInfo(const ExtractionZone &ExtZone) {
return true;
}
+ // Set by VisitMemberExpr right before traversing into a non-arrow
+ // MemberExpr's base, when that base is (possibly parenthesized)
+ // exactly a DeclRefExpr: since that base is always traversed
+ // immediately afterwards (it's the MemberExpr's only child),
+ // VisitDeclRefExpr can rely on this still describing itself, and
+ // must always consume (reset) it, matched or not, so it never leaks
+ // into an unrelated, later DeclRefExpr.
+ std::optional<SourceLocation> PendingMemberDotLoc;
+
+ bool VisitMemberExpr(MemberExpr *ME) {
+ if (!ME->isArrow() && isa<DeclRefExpr>(ME->getBase()->IgnoreParens()))
+ PendingMemberDotLoc = ME->getOperatorLoc();
+ return true;
+ }
+
bool VisitDeclRefExpr(DeclRefExpr *DRE) {
// Find the corresponding Decl and mark it's occurrence.
const Decl *D = DRE->getDecl();
@@ -764,6 +862,10 @@ CapturedZoneInfo captureZoneInfo(const ExtractionZone &ExtZone) {
if (!DeclInfo)
DeclInfo = Info.createDeclInfo(D, ZoneRelative::OutsideFunc);
DeclInfo->markOccurence(CurrentLocation);
+ if (CurrentLocation == ZoneRelative::Inside)
+ DeclInfo->ZoneOccurrences.push_back(
+ {DRE->getLocation(), PendingMemberDotLoc});
+ PendingMemberDotLoc.reset();
return true;
}
@@ -993,7 +1095,7 @@ CapturedZoneInfo captureZoneInfo(const ExtractionZone &ExtZone) {
// FIXME: Check if the declaration has a local/anonymous type
bool createParameters(NewFunction &ExtractedFunc,
const CapturedZoneInfo &CapturedInfo,
- const ASTContext &Context) {
+ const ASTContext &Context, const LangOptions &LangOpts) {
for (const auto &KeyVal : CapturedInfo.DeclInfoMap) {
const auto &DeclInfo = KeyVal.second;
// If a Decl was Declared in zone and referenced in post zone, it
@@ -1017,7 +1119,7 @@ bool createParameters(NewFunction &ExtractedFunc,
QualType FullTypeInfo = VD->getType();
QualType TypeInfo = FullTypeInfo.getNonReferenceType();
// FIXME: check if parameter will be a non l-value reference.
- bool IsPassedByReference = true;
+ ParamPassKind Kind = ParamPassKind::Reference;
if (!DeclInfo.IsPossiblyMutated) {
auto WordSize = Context.getTypeSizeInChars(Context.VoidPtrTy);
// A scalar (arithmetic, pointer, enumeration, ...) is at least as
@@ -1025,7 +1127,7 @@ bool createParameters(NewFunction &ExtractedFunc,
if (TypeInfo->isScalarType() && !TypeInfo.isVolatileQualified() &&
!FullTypeInfo->isReferenceType() &&
Context.getTypeSizeInChars(TypeInfo) <= 2 * WordSize) {
- IsPassedByReference = false;
+ Kind = ParamPassKind::Value;
} 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:
@@ -1035,10 +1137,39 @@ bool createParameters(NewFunction &ExtractedFunc,
TypeInfo.addConst();
}
}
+
+ // C has no references. A parameter that would otherwise need one
+ // becomes a real pointer instead: the call site takes the address of
+ // the original variable explicitly, and every use of it in the
+ // copied-out body is rewritten into a dereference (see
+ // NewFunction::getFuncBody). Array types are the exception: they decay
+ // to a pointer on their own wherever they're used, so no rewriting or
+ // address-of is needed for them at all.
+ if (Kind == ParamPassKind::Reference && !LangOpts.CPlusPlus) {
+ if (TypeInfo->isArrayType()) {
+ Kind = ParamPassKind::Value;
+ } else {
+ // Bail out rather than rewrite a use whose location can't be
+ // mapped back to a single, unambiguous spot in the source (e.g.
+ // one produced by macro expansion).
+ if (llvm::any_of(
+ DeclInfo.ZoneOccurrences,
+ [](const CapturedZoneInfo::DeclInformation::Occurrence &O) {
+ return O.Loc.isMacroID();
+ }))
+ return false;
+ TypeInfo = Context.getPointerType(TypeInfo);
+ Kind = ParamPassKind::Pointer;
+ unsigned NameLength = VD->getName().size();
+ for (const auto &Occ : DeclInfo.ZoneOccurrences)
+ ExtractedFunc.PointerRewriteSites.push_back(
+ {Occ.Loc, NameLength, Occ.DotLoc});
+ }
+ }
+
// We use the index of declaration as the ordering priority for parameters.
- ExtractedFunc.Parameters.push_back({std::string(VD->getName()), TypeInfo,
- IsPassedByReference,
- DeclInfo.DeclIndex});
+ ExtractedFunc.Parameters.push_back(
+ {std::string(VD->getName()), TypeInfo, Kind, DeclInfo.DeclIndex});
}
llvm::sort(ExtractedFunc.Parameters);
return true;
@@ -1134,7 +1265,7 @@ llvm::Expected<NewFunction> getExtractedFunction(ExtractionZone &ExtZone,
ExtractedFunc.CallerReturnsValue = CapturedInfo.AlwaysReturns;
if (!createParameters(ExtractedFunc, CapturedInfo,
- ExtZone.EnclosingFunction->getASTContext()) ||
+ ExtZone.EnclosingFunction->getASTContext(), LangOpts) ||
!generateReturnProperties(ExtractedFunc, *ExtZone.EnclosingFunction,
CapturedInfo))
return error("Too complex to extract.");
@@ -1209,8 +1340,6 @@ bool hasReturnStmt(const ExtractionZone &ExtZone) {
bool ExtractFunction::prepare(const Selection &Inputs) {
const LangOptions &LangOpts = Inputs.AST->getLangOpts();
- if (!LangOpts.CPlusPlus)
- return false;
const Node *CommonAnc = Inputs.ASTSelection.commonAncestor();
const SourceManager &SM = Inputs.AST->getSourceManager();
auto MaybeExtZone = findExtractionZone(CommonAnc, SM, LangOpts);
diff --git a/clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp b/clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp
index 0f20fb218d07a..f13c96c59ba77 100644
--- a/clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp
+++ b/clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp
@@ -10,6 +10,7 @@
#include "gmock/gmock.h"
#include "gtest/gtest.h"
+using ::testing::AllOf;
using ::testing::HasSubstr;
using ::testing::Not;
using ::testing::StartsWith;
@@ -56,9 +57,6 @@ TEST_F(ExtractFunctionTest, FunctionTest) {
EXPECT_THAT(
apply("#define RETURN_IF_ERROR(x) if (x) return\nRETU^RN_IF_ERROR(4);"),
StartsWith("unavailable"));
-
- FileName = "a.c";
- EXPECT_THAT(apply(" for([[int i = 0;]];);"), HasSubstr("unavailable"));
}
TEST_F(ExtractFunctionTest, FileTest) {
@@ -1044,6 +1042,82 @@ TEST_F(ExtractFunctionTest, VolatileScalar) {
HasSubstr("extracted(const volatile int &V)"));
}
+TEST_F(ExtractFunctionTest, CFileAllowUnmodifiedScalar) {
+ FileName = "a.c";
+ Context = File;
+ EXPECT_THAT(apply(R"cpp(
+ int i;
+ void foo() {
+ int j = 0;
+ [[i = j;]]
+ })cpp"),
+ HasSubstr("extracted(int j)"));
+}
+
+TEST_F(ExtractFunctionTest, CFileModifiedScalarBecomesPointer) {
+ // C has no references: a mutated capture becomes a real pointer
+ // parameter instead, with the call site taking its address and the
+ // body dereferencing it.
+ FileName = "a.c";
+ Context = File;
+ EXPECT_THAT(apply(R"cpp(
+ void foo() {
+ int j;
+ [[j = 0;]]
+ })cpp"),
+ AllOf(HasSubstr("extracted(int * j)"), HasSubstr("(*j) = 0;"),
+ HasSubstr("extracted(&j)")));
+}
+
+TEST_F(ExtractFunctionTest, CFileUnmodifiedStructBecomesConstPointer) {
+ // Same, but for an unmutated non-scalar capture: the parameter becomes
+ // a pointer to const, and every member access on it is rewritten too.
+ FileName = "a.c";
+ Context = File;
+ EXPECT_THAT(apply(R"cpp(
+ struct pair { int v1; int v2; };
+ int i;
+ void foo() {
+ struct pair p;
+ p.v1 = 0;
+ [[i = p.v1;]]
+ })cpp"),
+ AllOf(HasSubstr("extracted(const struct pair * p)"),
+ HasSubstr("i = p->v1;"), HasSubstr("extracted(&p)")));
+}
+
+TEST_F(ExtractFunctionTest, CFileStructMixedUses) {
+ // The same capture can appear both as a member-access base (rewritten
+ // to "->") and as a plain use (wrapped in "(*...)") within a single
+ // extraction; each occurrence is rewritten independently.
+ FileName = "a.c";
+ Context = File;
+ EXPECT_THAT(apply(R"cpp(
+ struct pair { int v1; int v2; };
+ void use(struct pair);
+ int i;
+ void foo() {
+ struct pair p;
+ [[use(p); i = p.v1;]]
+ })cpp"),
+ AllOf(HasSubstr("use((*p));"), HasSubstr("i = p->v1;")));
+}
+
+TEST_F(ExtractFunctionTest, CFileModifiedArrayStaysPlainPointer) {
+ // Unlike other non-scalar types, an array decays to a pointer on its
+ // own wherever it's used, so it needs neither an address-of at the
+ // call site nor a dereference-rewrite of its uses in the body.
+ FileName = "a.c";
+ Context = File;
+ EXPECT_THAT(apply(R"cpp(
+ void foo() {
+ int arr[5];
+ [[arr[0] = 1;]]
+ })cpp"),
+ AllOf(HasSubstr("arr[0] = 1;"), HasSubstr("extracted(arr)"),
+ Not(HasSubstr("&arr"))));
+}
+
} // namespace
} // namespace clangd
} // namespace clang
>From efc7cb376b1d3b53b59566e885ac70424c4bad85 Mon Sep 17 00:00:00 2001
From: Christian Kandeler <christian.kandeler at qt.io>
Date: Tue, 6 Oct 2026 17:03:46 +0200
Subject: [PATCH 2/4] [clangd] Extract to function: Fix C pointer-adapter
follow-up issues
Array-typed parameters were left with their original array type, which
NewFunction::Parameter::render() prints as the uncompilable "int[5]
name" instead of decaying it to a pointer. Decaying an array to a
pointer would also break sizeof/alignof/typeof (and spelling variants)
on it, since those would then observe the pointer's properties instead
of the array's; such uses are now refused instead of risking silently
wrong code.
A free function's own `static` (internal linkage) wasn't carried over
to an extracted sibling function, unlike for methods.
getFuncBody's manual buffer-offset splicing is replaced with
tooling::Replacements, with any failure now propagated as a genuine
extraction failure instead of silently producing wrong output.
---
.../refactor/tweaks/ExtractFunction.cpp | 192 ++++++++++++------
.../unittests/tweaks/ExtractFunctionTests.cpp | 66 +++++-
2 files changed, 196 insertions(+), 62 deletions(-)
diff --git a/clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp b/clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp
index d98ba07e7a49e..c8ec670aeac8d 100644
--- a/clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp
+++ b/clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp
@@ -486,10 +486,10 @@ struct NewFunction {
// Render the call for this function.
std::string renderCall() const;
// Render the definition for this function.
- std::string renderDeclaration(FunctionDeclKind K,
- const DeclContext &SemanticDC,
- const DeclContext &SyntacticDC,
- const SourceManager &SM) const;
+ llvm::Expected<std::string> renderDeclaration(FunctionDeclKind K,
+ const DeclContext &SemanticDC,
+ const DeclContext &SyntacticDC,
+ const SourceManager &SM) const;
private:
std::string
@@ -499,7 +499,7 @@ struct NewFunction {
std::string renderQualifiers() const;
std::string renderDeclarationName(FunctionDeclKind K) const;
// Generate the function body.
- std::string getFuncBody(const SourceManager &SM) const;
+ llvm::Expected<std::string> getFuncBody(const SourceManager &SM) const;
};
std::string NewFunction::renderParametersForDeclaration(
@@ -578,10 +578,9 @@ std::string NewFunction::renderCall() const {
(SemicolonPolicy.isNeededInOriginalFunction() ? ";" : "")));
}
-std::string NewFunction::renderDeclaration(FunctionDeclKind K,
- const DeclContext &SemanticDC,
- const DeclContext &SyntacticDC,
- const SourceManager &SM) const {
+llvm::Expected<std::string> NewFunction::renderDeclaration(
+ FunctionDeclKind K, const DeclContext &SemanticDC,
+ const DeclContext &SyntacticDC, const SourceManager &SM) const {
std::string Declaration = std::string(llvm::formatv(
"{0}{1} {2}({3}){4}", renderSpecifiers(K),
printType(ReturnType, SyntacticDC), renderDeclarationName(K),
@@ -591,53 +590,55 @@ std::string NewFunction::renderDeclaration(FunctionDeclKind K,
case ForwardDeclaration:
return std::string(llvm::formatv("{0};\n", Declaration));
case OutOfLineDefinition:
- case InlineDefinition:
- return std::string(
- llvm::formatv("{0} {\n{1}\n}\n", Declaration, getFuncBody(SM)));
- break;
+ case InlineDefinition: {
+ llvm::Expected<std::string> Body = getFuncBody(SM);
+ if (!Body)
+ return Body.takeError();
+ return std::string(llvm::formatv("{0} {\n{1}\n}\n", Declaration, *Body));
+ }
}
llvm_unreachable("Unsupported FunctionDeclKind enum");
}
-std::string NewFunction::getFuncBody(const SourceManager &SM) const {
+llvm::Expected<std::string>
+NewFunction::getFuncBody(const SourceManager &SM) const {
// FIXME: Generate tooling::Replacements instead of std::string to
// - hoist decls
// - add return statement
// - Add semicolon
- std::string Body;
- if (PointerRewriteSites.empty()) {
- Body = toSourceCode(SM, BodyRange).str();
- } else {
- // Splice in each site's rewrite, keeping everything else verbatim.
- auto Sites = PointerRewriteSites;
- llvm::sort(Sites, [&SM](const auto &A, const auto &B) {
- return SM.isBeforeInTranslationUnit(A.Loc, B.Loc);
- });
- FileID FID = SM.getFileID(BodyRange.getBegin());
- StringRef Buf = SM.getBufferOrFake(FID).getBuffer();
- unsigned Cursor = SM.getFileOffset(BodyRange.getBegin());
- for (const auto &Site : Sites) {
- unsigned SiteBegin = SM.getFileOffset(Site.Loc);
- Body += Buf.substr(Cursor, SiteBegin - Cursor);
- if (Site.DotLoc) {
- // Leave the identifier itself untouched; turn the member access
- // that follows it into "->" instead of wrapping in a dereference.
- Body += Buf.substr(SiteBegin, Site.NameLength);
- Cursor = SiteBegin + Site.NameLength;
- unsigned DotOffset = SM.getFileOffset(*Site.DotLoc);
- Body += Buf.substr(Cursor, DotOffset - Cursor);
- Body += "->";
- Cursor = DotOffset + 1;
- } else {
- Body += "(*";
- Body += Buf.substr(SiteBegin, Site.NameLength);
- Body += ")";
- Cursor = SiteBegin + Site.NameLength;
- }
+ std::string Body = toSourceCode(SM, BodyRange).str();
+ if (PointerRewriteSites.empty())
+ return Body + (SemicolonPolicy.isNeededInExtractedFunction() ? ";" : "");
+
+ // Splice in each site's rewrite. These are all independent
+ // insertions/single-token replacements at distinct, non-overlapping
+ // source locations (two DeclRefExprs can't share a location, and a dot
+ // always follows its own identifier), so none of this should actually
+ // be able to fail -- but if that assumption is ever wrong, surface it
+ // as an extraction failure instead of silently emitting broken code.
+ tooling::Replacements Repls;
+ unsigned BodyBeginOffset = SM.getFileOffset(BodyRange.getBegin());
+ for (const auto &Site : PointerRewriteSites) {
+ unsigned SiteOffset = SM.getFileOffset(Site.Loc) - BodyBeginOffset;
+ if (Site.DotLoc) {
+ // Leave the identifier itself untouched; turn the member access
+ // that follows it into "->" instead of wrapping in a dereference.
+ unsigned DotOffset = SM.getFileOffset(*Site.DotLoc) - BodyBeginOffset;
+ if (auto Err = Repls.add(tooling::Replacement("", DotOffset, 1, "->")))
+ return std::move(Err);
+ } else {
+ if (auto Err = Repls.add(tooling::Replacement("", SiteOffset, 0, "(*")))
+ return std::move(Err);
+ if (auto Err = Repls.add(
+ tooling::Replacement("", SiteOffset + Site.NameLength, 0, ")")))
+ return std::move(Err);
}
- Body += Buf.substr(Cursor, SM.getFileOffset(BodyRange.getEnd()) - Cursor);
}
- return Body + (SemicolonPolicy.isNeededInExtractedFunction() ? ";" : "");
+ llvm::Expected<std::string> NewBody =
+ tooling::applyAllReplacements(Body, Repls);
+ if (!NewBody)
+ return NewBody.takeError();
+ return *NewBody + (SemicolonPolicy.isNeededInExtractedFunction() ? ";" : "");
}
std::string NewFunction::Parameter::render(const DeclContext *Context) const {
@@ -674,6 +675,17 @@ struct CapturedZoneInfo {
std::optional<SourceLocation> DotLoc;
};
llvm::SmallVector<Occurrence, 1> ZoneOccurrences;
+ // Whether this Decl is ever the direct operand of a trait that
+ // depends on its exact, undecayed type: `sizeof`/`alignof`/`typeof`
+ // and their spelling variants (e.g. `sizeof arr`/`sizeof(arr)`, as
+ // opposed to `sizeof(T)`, which doesn't reference arr's Decl at all).
+ // If this Decl is an array and ends up decaying to a pointer for a C
+ // pointer-adapter parameter (see createParameters), such a use would
+ // silently start reporting the pointer's properties instead of the
+ // array's (e.g. 8 instead of 20 for `sizeof` on a 5-element `int`
+ // array on a 64-bit target), so that decay is refused whenever this
+ // is set.
+ bool HasUnsafeTypeQueryUseInZone = false;
DeclInformation(const Decl *TheDecl, ZoneRelative DeclaredIn,
unsigned DeclIndex)
: TheDecl(TheDecl), DeclaredIn(DeclaredIn), DeclIndex(DeclIndex){};
@@ -854,6 +866,39 @@ CapturedZoneInfo captureZoneInfo(const ExtractionZone &ExtZone) {
return true;
}
+ // Marks D as having an unsafe, type-dependent use in the zone (see
+ // DeclInformation::HasUnsafeTypeQueryUseInZone) if it's reached
+ // through a plain (possibly parenthesized) DeclRefExpr -- e.g. not
+ // through a cast, which would already observe the decayed type rather
+ // than the Decl's own declared type.
+ void markUnsafeTypeQueryOperand(const Expr *E) {
+ if (CurrentLocation != ZoneRelative::Inside)
+ return;
+ if (const auto *DRE = dyn_cast<DeclRefExpr>(E->IgnoreParens()))
+ if (auto *DeclInfo = Info.getDeclInfoFor(DRE->getDecl()))
+ DeclInfo->HasUnsafeTypeQueryUseInZone = true;
+ }
+
+ // Covers sizeof/alignof/__alignof/_Countof and their variants: all
+ // share this one node type, distinguished only by getKind().
+ bool VisitUnaryExprOrTypeTraitExpr(UnaryExprOrTypeTraitExpr *E) {
+ if (!E->isArgumentType())
+ markUnsafeTypeQueryOperand(E->getArgumentExpr());
+ return true;
+ }
+
+ // Covers typeof/typeof_unqual/__typeof__ (decltype itself is C++
+ // only, so can't appear in the C code this matters for, but is
+ // included for completeness/robustness).
+ bool VisitTypeOfExprTypeLoc(TypeOfExprTypeLoc TL) {
+ markUnsafeTypeQueryOperand(TL.getUnderlyingExpr());
+ return true;
+ }
+ bool VisitDecltypeTypeLoc(DecltypeTypeLoc TL) {
+ markUnsafeTypeQueryOperand(TL.getUnderlyingExpr());
+ return true;
+ }
+
bool VisitDeclRefExpr(DeclRefExpr *DRE) {
// Find the corresponding Decl and mark it's occurrence.
const Decl *D = DRE->getDecl();
@@ -1144,9 +1189,18 @@ bool createParameters(NewFunction &ExtractedFunc,
// copied-out body is rewritten into a dereference (see
// NewFunction::getFuncBody). Array types are the exception: they decay
// to a pointer on their own wherever they're used, so no rewriting or
- // address-of is needed for them at all.
+ // address-of is needed for them at all -- except that TypeInfo itself
+ // must be decayed too (NewFunction::Parameter::render() has no special
+ // case for array declarator syntax, so leaving it as an array type
+ // would print as the uncompilable `int[5] name`).
if (Kind == ParamPassKind::Reference && !LangOpts.CPlusPlus) {
if (TypeInfo->isArrayType()) {
+ // A decayed pointer no longer reports the array's own size,
+ // alignment, or type: bail out rather than silently break a
+ // sizeof/alignof/typeof (or similar) on it.
+ if (DeclInfo.HasUnsafeTypeQueryUseInZone)
+ return false;
+ TypeInfo = Context.getArrayDecayedType(TypeInfo);
Kind = ParamPassKind::Value;
} else {
// Bail out rather than rewrite a use whose location can't be
@@ -1243,6 +1297,12 @@ llvm::Expected<NewFunction> getExtractedFunction(ExtractionZone &ExtZone,
ExtractedFunc.DefinitionQualifier = ExtZone.EnclosingFunction->getQualifier();
ExtractedFunc.Constexpr = ExtZone.EnclosingFunction->getConstexprKind();
+ // A free function declared `static` has internal linkage: an extracted
+ // sibling should keep that, or it'd default to external linkage instead.
+ // For a method, this gets overridden just below by the more precise
+ // (and differently-meaning) CXXMethodDecl::isStatic().
+ ExtractedFunc.Static =
+ ExtZone.EnclosingFunction->getStorageClass() == SC_Static;
if (const auto *Method =
llvm::dyn_cast<CXXMethodDecl>(ExtZone.EnclosingFunction))
captureMethodInfo(ExtractedFunc, Method);
@@ -1295,26 +1355,32 @@ tooling::Replacement replaceWithFuncCall(const NewFunction &ExtractedFunc,
SM, CharSourceRange(ExtractedFunc.BodyRange, false), FuncCall, LangOpts);
}
-tooling::Replacement createFunctionDefinition(const NewFunction &ExtractedFunc,
- const SourceManager &SM) {
+llvm::Expected<tooling::Replacement>
+createFunctionDefinition(const NewFunction &ExtractedFunc,
+ const SourceManager &SM) {
FunctionDeclKind DeclKind = InlineDefinition;
if (ExtractedFunc.ForwardDeclarationPoint)
DeclKind = OutOfLineDefinition;
- std::string FunctionDef = ExtractedFunc.renderDeclaration(
+ llvm::Expected<std::string> FunctionDef = ExtractedFunc.renderDeclaration(
DeclKind, *ExtractedFunc.SemanticDC, *ExtractedFunc.SyntacticDC, SM);
+ if (!FunctionDef)
+ return FunctionDef.takeError();
return tooling::Replacement(SM, ExtractedFunc.DefinitionPoint, 0,
- FunctionDef);
+ *FunctionDef);
}
-tooling::Replacement createForwardDeclaration(const NewFunction &ExtractedFunc,
- const SourceManager &SM) {
- std::string FunctionDecl = ExtractedFunc.renderDeclaration(
+llvm::Expected<tooling::Replacement>
+createForwardDeclaration(const NewFunction &ExtractedFunc,
+ const SourceManager &SM) {
+ llvm::Expected<std::string> FunctionDecl = ExtractedFunc.renderDeclaration(
ForwardDeclaration, *ExtractedFunc.SemanticDC,
*ExtractedFunc.ForwardDeclarationSyntacticDC, SM);
+ if (!FunctionDecl)
+ return FunctionDecl.takeError();
SourceLocation DeclPoint = *ExtractedFunc.ForwardDeclarationPoint;
- return tooling::Replacement(SM, DeclPoint, 0, FunctionDecl);
+ return tooling::Replacement(SM, DeclPoint, 0, *FunctionDecl);
}
// Returns true if ExtZone contains any ReturnStmts.
@@ -1363,7 +1429,10 @@ Expected<Tweak::Effect> ExtractFunction::apply(const Selection &Inputs) {
if (!ExtractedFunc)
return ExtractedFunc.takeError();
tooling::Replacements Edit;
- if (auto Err = Edit.add(createFunctionDefinition(*ExtractedFunc, SM)))
+ auto FuncDef = createFunctionDefinition(*ExtractedFunc, SM);
+ if (!FuncDef)
+ return FuncDef.takeError();
+ if (auto Err = Edit.add(*FuncDef))
return std::move(Err);
if (auto Err = Edit.add(replaceWithFuncCall(*ExtractedFunc, SM, LangOpts)))
return std::move(Err);
@@ -1372,15 +1441,20 @@ Expected<Tweak::Effect> ExtractFunction::apply(const Selection &Inputs) {
// If the fwd-declaration goes in the same file, merge into Replacements.
// Otherwise it needs to be a separate file edit.
if (SM.isWrittenInSameFile(ExtractedFunc->DefinitionPoint, *FwdLoc)) {
- if (auto Err = Edit.add(createForwardDeclaration(*ExtractedFunc, SM)))
+ auto FwdDecl = createForwardDeclaration(*ExtractedFunc, SM);
+ if (!FwdDecl)
+ return FwdDecl.takeError();
+ if (auto Err = Edit.add(*FwdDecl))
return std::move(Err);
} else {
auto MultiFileEffect = Effect::mainFileEdit(SM, std::move(Edit));
if (!MultiFileEffect)
return MultiFileEffect.takeError();
- tooling::Replacements OtherEdit(
- createForwardDeclaration(*ExtractedFunc, SM));
+ auto FwdDecl = createForwardDeclaration(*ExtractedFunc, SM);
+ if (!FwdDecl)
+ return FwdDecl.takeError();
+ tooling::Replacements OtherEdit(*FwdDecl);
if (auto PathAndEdit =
Tweak::Effect::fileEdit(SM, SM.getFileID(*FwdLoc), OtherEdit))
MultiFileEffect->ApplyEdits.try_emplace(PathAndEdit->first,
diff --git a/clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp b/clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp
index f13c96c59ba77..ede57e30cd99f 100644
--- a/clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp
+++ b/clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp
@@ -1106,7 +1106,10 @@ TEST_F(ExtractFunctionTest, CFileStructMixedUses) {
TEST_F(ExtractFunctionTest, CFileModifiedArrayStaysPlainPointer) {
// Unlike other non-scalar types, an array decays to a pointer on its
// own wherever it's used, so it needs neither an address-of at the
- // call site nor a dereference-rewrite of its uses in the body.
+ // call site nor a dereference-rewrite of its uses in the body. The
+ // parameter's own type must be decayed too, though: leaving it as an
+ // array type would print as the uncompilable "int[5] arr" (there's no
+ // special-cased array declarator syntax, unlike C++'s reference case).
FileName = "a.c";
Context = File;
EXPECT_THAT(apply(R"cpp(
@@ -1114,8 +1117,65 @@ TEST_F(ExtractFunctionTest, CFileModifiedArrayStaysPlainPointer) {
int arr[5];
[[arr[0] = 1;]]
})cpp"),
- AllOf(HasSubstr("arr[0] = 1;"), HasSubstr("extracted(arr)"),
- Not(HasSubstr("&arr"))));
+ AllOf(HasSubstr("extracted(int * arr)"), HasSubstr("arr[0] = 1;"),
+ HasSubstr("extracted(arr)"), Not(HasSubstr("&arr"))));
+}
+
+TEST_F(ExtractFunctionTest, CFileStaticFunctionStaysStatic) {
+ // A free function's own `static` (internal linkage) must carry over to
+ // an extracted sibling, or that sibling would default to external
+ // linkage instead.
+ FileName = "a.c";
+ Context = File;
+ EXPECT_THAT(apply(R"cpp(
+ static void foo() {
+ int j = 0;
+ [[int k = j;]]
+ })cpp"),
+ HasSubstr("static void extracted"));
+}
+
+TEST_F(ExtractFunctionTest, CFileRejectArraySizeof) {
+ // Decaying the array to a pointer parameter would silently change the
+ // meaning of a `sizeof` on it (pointer size instead of array size), so
+ // this is refused rather than risk miscompiling it.
+ FileName = "a.c";
+ Context = File;
+ EXPECT_EQ(apply(R"cpp(
+ void foo() {
+ int arr[5];
+ [[int n = sizeof(arr);]]
+ })cpp"),
+ "fail: Too complex to extract.");
+}
+
+TEST_F(ExtractFunctionTest, CFileRejectArrayAlignof) {
+ // Same hazard as sizeof, and the same UnaryExprOrTypeTraitExpr AST
+ // node: alignof(int) and alignof(int *) aren't guaranteed to match
+ // (and commonly don't, e.g. 4 vs 8 on a typical 64-bit target).
+ FileName = "a.c";
+ Context = File;
+ EXPECT_EQ(apply(R"cpp(
+ void foo() {
+ int arr[5];
+ [[int n = __alignof(arr);]]
+ })cpp"),
+ "fail: Too complex to extract.");
+}
+
+TEST_F(ExtractFunctionTest, CFileRejectArrayTypeof) {
+ // Same hazard again, but via a completely different AST node
+ // (TypeOfExprType, reached through the VarDecl's TypeLoc, not through
+ // any Stmt a plain expression visitor would see): typeof(arr) would
+ // resolve to the decayed pointer type instead of the array type.
+ FileName = "a.c";
+ Context = File;
+ EXPECT_EQ(apply(R"cpp(
+ void foo() {
+ int arr[5];
+ [[__typeof__(arr) copy;]]
+ })cpp"),
+ "fail: Too complex to extract.");
}
} // namespace
>From 7b5df334f1eb55d27145672b0c0920bc3445e8cf Mon Sep 17 00:00:00 2001
From: Christian Kandeler <christian.kandeler at qt.io>
Date: Tue, 6 Oct 2026 17:03:50 +0200
Subject: [PATCH 3/4] [clangd] Document C support for Extract to function in
release notes
---
clang-tools-extra/docs/ReleaseNotes.md | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/clang-tools-extra/docs/ReleaseNotes.md b/clang-tools-extra/docs/ReleaseNotes.md
index 41aa783cabd24..3cf60f3433907 100644
--- a/clang-tools-extra/docs/ReleaseNotes.md
+++ b/clang-tools-extra/docs/ReleaseNotes.md
@@ -97,6 +97,11 @@ infrastructure are described first, followed by tool-specific sections.
operator call such as `stream << 42;`), which it previously refused to
extract.
+- The `Extract to function` tweak is now also available in C files, where
+ it previously always refused to apply. Captured variables are passed by
+ value when possible, and otherwise via a pointer parameter, since C has
+ no references.
+
#### Signature help
- Parameters declared with a `decltype` are now displayed as the type the
>From 6b9507601aa35279f34711c7df4cbd3969490f2c Mon Sep 17 00:00:00 2001
From: Christian Kandeler <christian.kandeler at qt.io>
Date: Wed, 7 Oct 2026 11:57:21 +0200
Subject: [PATCH 4/4] [clangd] Extract to function: Fix macro-dot handling and
static-function detection
A macro expanding to "." (e.g. `#define DOT .`) wasn't caught by the
existing macro-occurrence bail-out in the C pointer-adapter rewrite:
only the identifier's own location was checked, not the dot's, even
though the dot is a separate token that can independently come from a
macro expansion while the identifier doesn't.
Separately (and not specific to C, or to the pointer-adapter work at
all): a static function definition following an earlier `static`
forward declaration, but not itself repeating `static` (legal once a
prior declaration establishes internal linkage), wasn't recognized as
static, since only the current declaration's storage class was
checked rather than the canonical one.
---
.../refactor/tweaks/ExtractFunction.cpp | 22 +++++++++----
.../unittests/tweaks/ExtractFunctionTests.cpp | 33 +++++++++++++++++++
2 files changed, 48 insertions(+), 7 deletions(-)
diff --git a/clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp b/clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp
index c8ec670aeac8d..1ac049b4a4f0e 100644
--- a/clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp
+++ b/clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp
@@ -1203,13 +1203,17 @@ bool createParameters(NewFunction &ExtractedFunc,
TypeInfo = Context.getArrayDecayedType(TypeInfo);
Kind = ParamPassKind::Value;
} else {
- // Bail out rather than rewrite a use whose location can't be
- // mapped back to a single, unambiguous spot in the source (e.g.
- // one produced by macro expansion).
+ // Bail out rather than rewrite a use whose location (or, for a
+ // member-access rewrite, whose dot's location) can't be mapped
+ // back to a single, unambiguous spot in the source (e.g. one
+ // produced by macro expansion) -- the dot can be a macro
+ // expansion even when the identifier itself isn't, e.g.
+ // `#define DOT .` used as `s DOT x`.
if (llvm::any_of(
DeclInfo.ZoneOccurrences,
[](const CapturedZoneInfo::DeclInformation::Occurrence &O) {
- return O.Loc.isMacroID();
+ return O.Loc.isMacroID() ||
+ (O.DotLoc && O.DotLoc->isMacroID());
}))
return false;
TypeInfo = Context.getPointerType(TypeInfo);
@@ -1299,10 +1303,14 @@ llvm::Expected<NewFunction> getExtractedFunction(ExtractionZone &ExtZone,
// A free function declared `static` has internal linkage: an extracted
// sibling should keep that, or it'd default to external linkage instead.
- // For a method, this gets overridden just below by the more precise
- // (and differently-meaning) CXXMethodDecl::isStatic().
+ // Checked on the canonical (first) declaration, not this one: a
+ // definition following an earlier `static` forward declaration doesn't
+ // need to (and often doesn't) repeat `static` itself, but is still
+ // static. For a method, this gets overridden just below by the more
+ // precise (and differently-meaning) CXXMethodDecl::isStatic().
ExtractedFunc.Static =
- ExtZone.EnclosingFunction->getStorageClass() == SC_Static;
+ ExtZone.EnclosingFunction->getCanonicalDecl()->getStorageClass() ==
+ SC_Static;
if (const auto *Method =
llvm::dyn_cast<CXXMethodDecl>(ExtZone.EnclosingFunction))
captureMethodInfo(ExtractedFunc, Method);
diff --git a/clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp b/clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp
index ede57e30cd99f..b432f6b090c96 100644
--- a/clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp
+++ b/clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp
@@ -1103,6 +1103,23 @@ TEST_F(ExtractFunctionTest, CFileStructMixedUses) {
AllOf(HasSubstr("use((*p));"), HasSubstr("i = p->v1;")));
}
+TEST_F(ExtractFunctionTest, CFileRejectMacroDot) {
+ // The identifier itself need not be a macro expansion for the
+ // member-access ".", immediately following it, to be one -- that dot
+ // is a separate token with its own location, which also needs
+ // checking before relying on it to splice in "->".
+ FileName = "a.c";
+ Context = File;
+ EXPECT_EQ(apply(R"cpp(
+ #define DOT .
+ struct pair { int v1; int v2; };
+ void foo() {
+ struct pair p;
+ [[p DOT v1 = 1;]]
+ })cpp"),
+ "fail: Too complex to extract.");
+}
+
TEST_F(ExtractFunctionTest, CFileModifiedArrayStaysPlainPointer) {
// Unlike other non-scalar types, an array decays to a pointer on its
// own wherever it's used, so it needs neither an address-of at the
@@ -1135,6 +1152,22 @@ TEST_F(ExtractFunctionTest, CFileStaticFunctionStaysStatic) {
HasSubstr("static void extracted"));
}
+TEST_F(ExtractFunctionTest, CFileStaticForwardDeclaredFunctionStaysStatic) {
+ // Same as above, but the definition itself omits `static` (legal in C:
+ // once a prior declaration gives the function internal linkage, a
+ // later one doesn't need to repeat it, and still has it). Checking
+ // only the current declaration's storage class would miss this.
+ FileName = "a.c";
+ Context = File;
+ EXPECT_THAT(apply(R"cpp(
+ static void foo();
+ void foo() {
+ int j = 0;
+ [[int k = j;]]
+ })cpp"),
+ HasSubstr("static void extracted"));
+}
+
TEST_F(ExtractFunctionTest, CFileRejectArraySizeof) {
// Decaying the array to a pointer parameter would silently change the
// meaning of a `sizeof` on it (pointer size instead of array size), so
More information about the cfe-commits
mailing list