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 <[email protected]> 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 <[email protected]> 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 <[email protected]> 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 <[email protected]> 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 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
