https://github.com/ckandeler updated https://github.com/llvm/llvm-project/pull/230046
>From 60e6beac9a68d0f8d839fc85fc968797597b5f51 Mon Sep 17 00:00:00 2001 From: Christian Kandeler <[email protected]> Date: Wed, 7 Oct 2026 16:14:15 +0200 Subject: [PATCH 1/3] [clangd] Extract to function: Fix parameter declarator printing A parameter's type and name were rendered by printing the type as a standalone string and appending the name after it. That's wrong for any declarator syntax that doesn't simply put the name after the type: a function pointer parameter rendered as the uncompilable `void (*)(int) F` instead of `void (*F)(int)`, and a parameter that's a reference to an array rendered as the equally invalid `int[5] &arr` instead of `int (&arr)[5]`. Fixed by passing the name as printType()'s existing declarator placeholder argument instead, letting clang's own type printer place it correctly. For the reference case, this requires building the actual reference QualType rather than just prepending "&" to the placeholder string, since only the real type lets the printer parenthesize the array case correctly. Assisted-by: Claude Code --- .../refactor/tweaks/ExtractFunction.cpp | 16 +++++++++-- .../unittests/tweaks/ExtractFunctionTests.cpp | 28 +++++++++++++------ 2 files changed, 33 insertions(+), 11 deletions(-) diff --git a/clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp b/clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp index 1ac049b4a4f0ef..73bbef76206669 100644 --- a/clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp +++ b/clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp @@ -642,8 +642,20 @@ NewFunction::getFuncBody(const SourceManager &SM) const { } std::string NewFunction::Parameter::render(const DeclContext *Context) const { - return printType(TypeInfo, *Context) + - (Kind == ParamPassKind::Reference ? " &" : " ") + Name; + // Passing Name as printType()'s declarator placeholder (rather than just + // appending it after the printed type) lets clang's own type printer + // place it correctly for declarator syntax that doesn't simply put the + // name after the type, e.g. a function pointer (`void (*name)(int)`, not + // `void (*)(int) name`) or an array (`int name[5]`, not `int[5] name`). + // For a reference, building the actual reference QualType (rather than + // just prepending "&" to the placeholder string) is what lets the + // printer correctly parenthesize a reference-to-array + // (`int (&name)[5]`, not the invalid `int &name[5]`). + QualType RenderedType = + Kind == ParamPassKind::Reference + ? Context->getParentASTContext().getLValueReferenceType(TypeInfo) + : TypeInfo; + return printType(RenderedType, *Context, Name); } // Stores captured information about Extraction Zone. diff --git a/clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp b/clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp index b432f6b090c96a..43611ac9068653 100644 --- a/clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp +++ b/clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp @@ -79,7 +79,7 @@ void f(int a) { struct Foo { int x; }; -void extracted(int &a, int b, int * &ptr, const Foo &foo) { +void extracted(int &a, int b, int *&ptr, const Foo &foo) { a += foo.x + b; *ptr++; } @@ -927,12 +927,12 @@ TEST_F(ExtractFunctionTest, ConstParametersPointerIndirection) { struct S { int x; }; void f(S *ptr) { [[ptr->x = 1;]] } )cpp"), - HasSubstr("extracted(S * ptr)")); + HasSubstr("extracted(S *ptr)")); EXPECT_THAT(apply("void f(int *p) { [[p[0] = 1;]] }"), - HasSubstr("extracted(int * p)")); + HasSubstr("extracted(int *p)")); // Same for a plain dereference. EXPECT_THAT(apply("void f(int *p) { [[*p = 1;]] }"), - HasSubstr("extracted(int * p)")); + HasSubstr("extracted(int *p)")); } TEST_F(ExtractFunctionTest, ConstParametersConditionalReferenceBinding) { @@ -975,11 +975,21 @@ TEST_F(ExtractFunctionTest, ConstParametersStaticOperatorCall) { HasSubstr("extracted(const S &s, int x)")); } +TEST_F(ExtractFunctionTest, ConstParametersFunctionPointer) { + Context = File; + // A function pointer's declarator syntax puts the name inside the + // parens around the "*", not after the whole type: printType()'s + // placeholder must be threaded through for this to render as + // `void (*F)(int)` rather than the uncompilable `void (*)(int) F`. + EXPECT_THAT(apply("void f(void (*F)(int)) { [[F(1);]] }"), + HasSubstr("extracted(void (*F)(int))")); +} + TEST_F(ExtractFunctionTest, ConstParametersScalarsByValue) { Context = File; // An unmutated pointer is a scalar too: passed by value. EXPECT_THAT(apply("void use(int *); void f(int *p) { [[use(p);]] }"), - HasSubstr("extracted(int * p)")); + HasSubstr("extracted(int *p)")); // An unmutated enum: passed by value. EXPECT_THAT(apply(R"cpp( enum E { A, B }; @@ -998,7 +1008,7 @@ TEST_F(ExtractFunctionTest, ConstParametersScalarsByValue) { // An unmutated array is not a scalar (even though its element type is): // stays a non-const reference, per the existing array carve-out. EXPECT_THAT(apply("void f() { int arr[5]; [[int x = arr[0];]] }"), - HasSubstr("extracted(int[5] &arr)")); + HasSubstr("extracted(int (&arr)[5])")); } // Variables of reference type, const or non-const, must stay references, @@ -1065,7 +1075,7 @@ TEST_F(ExtractFunctionTest, CFileModifiedScalarBecomesPointer) { int j; [[j = 0;]] })cpp"), - AllOf(HasSubstr("extracted(int * j)"), HasSubstr("(*j) = 0;"), + AllOf(HasSubstr("extracted(int *j)"), HasSubstr("(*j) = 0;"), HasSubstr("extracted(&j)"))); } @@ -1082,7 +1092,7 @@ TEST_F(ExtractFunctionTest, CFileUnmodifiedStructBecomesConstPointer) { p.v1 = 0; [[i = p.v1;]] })cpp"), - AllOf(HasSubstr("extracted(const struct pair * p)"), + AllOf(HasSubstr("extracted(const struct pair *p)"), HasSubstr("i = p->v1;"), HasSubstr("extracted(&p)"))); } @@ -1134,7 +1144,7 @@ TEST_F(ExtractFunctionTest, CFileModifiedArrayStaysPlainPointer) { int arr[5]; [[arr[0] = 1;]] })cpp"), - AllOf(HasSubstr("extracted(int * arr)"), HasSubstr("arr[0] = 1;"), + AllOf(HasSubstr("extracted(int *arr)"), HasSubstr("arr[0] = 1;"), HasSubstr("extracted(arr)"), Not(HasSubstr("&arr")))); } >From 601fdf0f7ca430cd1bbb9b697132b58beb8aed7f Mon Sep 17 00:00:00 2001 From: Christian Kandeler <[email protected]> Date: Fri, 9 Oct 2026 10:22:19 +0200 Subject: [PATCH 2/3] [clangd] Extract to function: Fix function-reference const and reject VLAs Adding const to an unmutated captured reference applied to a captured function reference too, producing unparseable output: a (non-pointer) function type can't be cv-qualified at all, so `void (&F)(int)` became the invalid `void (const &F)(int)`. Excluded function types from that branch, same as arrays already were for a different reason. In C, decaying a captured array to a pointer parameter also needs to reject variable-length arrays: a VLA's bound (at any nesting depth, e.g. the inner dimension of `int A[2][N]`) is an arbitrary expression, typically another variable's name, that isn't necessarily a parameter of the extracted function at all, and even when it is, the printed array-pointer parameter type can't correctly account for it. Assisted-by: Claude Code --- .../refactor/tweaks/ExtractFunction.cpp | 18 ++++++++++-- .../unittests/tweaks/ExtractFunctionTests.cpp | 28 +++++++++++++++++++ 2 files changed, 44 insertions(+), 2 deletions(-) diff --git a/clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp b/clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp index 73bbef76206669..672a6395c3a3a4 100644 --- a/clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp +++ b/clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp @@ -1185,12 +1185,15 @@ bool createParameters(NewFunction &ExtractedFunc, !FullTypeInfo->isReferenceType() && Context.getTypeSizeInChars(TypeInfo) <= 2 * WordSize) { Kind = ParamPassKind::Value; - } else if (!TypeInfo->isArrayType()) { + } else if (!TypeInfo->isArrayType() && !TypeInfo->isFunctionType()) { // Still passed by reference to avoid a copy, but the reference // doesn't need to be mutable. Array types are never made const: // mutating array elements through a non-const-ref loop variable // or a decayed pointer argument is common and easy to miss - // conservatively, so we don't try. + // conservatively, so we don't try. A (non-pointer) function type + // can't be cv-qualified at all -- doing so anyway produces + // unparseable output, e.g. `void (const &F)(int)` for a + // captured function reference. TypeInfo.addConst(); } } @@ -1212,6 +1215,17 @@ bool createParameters(NewFunction &ExtractedFunc, // sizeof/alignof/typeof (or similar) on it. if (DeclInfo.HasUnsafeTypeQueryUseInZone) return false; + // A variable-length array's bound (at any nesting depth, e.g. + // the inner dimension of `int A[2][N]`) is an arbitrary + // expression that's typically just some other variable's name + // -- but that variable isn't necessarily (and often isn't) also + // a parameter of the extracted function, and even if it is, a + // mutated one would itself have been turned into a pointer, + // which the bound expression can't account for either way. + // Bail out rather than risk printing a parameter type that + // references a name meaningless (or absent) in the new scope. + if (TypeInfo->isVariablyModifiedType()) + return false; TypeInfo = Context.getArrayDecayedType(TypeInfo); Kind = ParamPassKind::Value; } else { diff --git a/clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp b/clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp index 43611ac9068653..c0e2de9c7b95fa 100644 --- a/clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp +++ b/clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp @@ -985,6 +985,16 @@ TEST_F(ExtractFunctionTest, ConstParametersFunctionPointer) { HasSubstr("extracted(void (*F)(int))")); } +TEST_F(ExtractFunctionTest, ConstParametersFunctionReference) { + Context = File; + // A (non-pointer) function type can't be cv-qualified at all, unlike + // every other non-scalar, non-array type this conservative-const + // logic otherwise applies to: adding const here would produce + // unparseable output (`void (const &F)(int)`). + EXPECT_THAT(apply("void f(void (&F)(int)) { [[F(0);]] }"), + HasSubstr("extracted(void (&F)(int))")); +} + TEST_F(ExtractFunctionTest, ConstParametersScalarsByValue) { Context = File; // An unmutated pointer is a scalar too: passed by value. @@ -1221,6 +1231,24 @@ TEST_F(ExtractFunctionTest, CFileRejectArrayTypeof) { "fail: Too complex to extract."); } +TEST_F(ExtractFunctionTest, CFileRejectVariableLengthArray) { + // A VLA's bound is an arbitrary expression -- typically another + // variable's name -- that isn't necessarily (and here isn't at all) + // itself a parameter of the extracted function, and even printing it + // via the usual array-decay path would reference a name meaningless + // in the new scope. Here the VLA bound isn't even at the top level + // (it's the inner dimension of a 2D array), exercising that + // `isVariablyModifiedType()` catches it regardless of nesting depth. + FileName = "a.c"; + Context = File; + EXPECT_EQ(apply(R"cpp( + void foo(int N) { + int A[2][N]; + [[A[0][0] = 1;]] + })cpp"), + "fail: Too complex to extract."); +} + } // namespace } // namespace clangd } // namespace clang >From bc4113bf2dabf86d3f2582e5511bcb387b1a4e65 Mon Sep 17 00:00:00 2001 From: Christian Kandeler <[email protected]> Date: Fri, 9 Oct 2026 12:58:04 +0200 Subject: [PATCH 3/3] [clangd] Extract to function: Don't over-reject variable-length arrays Only an array whose decayed pointer type still has a variable dimension is actually a problem (e.g. `int A[2][N]` decays to `int (*A)[N]`, which can't correctly account for `N`). A single- dimension VLA decays to a plain pointer with no bound to print at all (`int A[N]` to `int *A`), so it was being rejected unnecessarily. Fixed by checking the decayed type's pointee for variability instead of the original array type. Also added a test confirming C++ already handles VLA captures correctly as-is: an unmutated array there always stays a reference, never decaying to a pointer, so there's no separate parameter that needs to agree with the array's own bound. Assisted-by: Claude Code --- .../refactor/tweaks/ExtractFunction.cpp | 27 ++++++----- .../unittests/tweaks/ExtractFunctionTests.cpp | 45 +++++++++++++++---- 2 files changed, 53 insertions(+), 19 deletions(-) diff --git a/clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp b/clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp index 672a6395c3a3a4..3077fc67ea3db5 100644 --- a/clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp +++ b/clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp @@ -1215,18 +1215,23 @@ bool createParameters(NewFunction &ExtractedFunc, // sizeof/alignof/typeof (or similar) on it. if (DeclInfo.HasUnsafeTypeQueryUseInZone) return false; - // A variable-length array's bound (at any nesting depth, e.g. - // the inner dimension of `int A[2][N]`) is an arbitrary - // expression that's typically just some other variable's name - // -- but that variable isn't necessarily (and often isn't) also - // a parameter of the extracted function, and even if it is, a - // mutated one would itself have been turned into a pointer, - // which the bound expression can't account for either way. - // Bail out rather than risk printing a parameter type that - // references a name meaningless (or absent) in the new scope. - if (TypeInfo->isVariablyModifiedType()) + QualType Decayed = Context.getArrayDecayedType(TypeInfo); + // The array's own outermost dimension is never a problem, VLA + // or not: once decayed, it no longer appears in the type at + // all (`int A[N]` decays to plain `int *A`). An inner + // dimension that's still variable-length (e.g. the `N` in + // `int A[2][N]`, which decays to `int (*A)[N]`) is the actual + // hazard: its bound is an arbitrary expression, typically just + // some other variable's name, that isn't necessarily (and + // often isn't) also a parameter of the extracted function, and + // even if it is, a mutated one would itself have been turned + // into a pointer, which the bound expression can't account for + // either way. Bail out rather than risk printing a parameter + // type that references a name meaningless (or absent) in the + // new scope. + if (Decayed->getPointeeType()->isVariablyModifiedType()) return false; - TypeInfo = Context.getArrayDecayedType(TypeInfo); + TypeInfo = Decayed; Kind = ParamPassKind::Value; } else { // Bail out rather than rewrite a use whose location (or, for a diff --git a/clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp b/clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp index c0e2de9c7b95fa..9857b8e64b1f99 100644 --- a/clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp +++ b/clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp @@ -1231,14 +1231,28 @@ TEST_F(ExtractFunctionTest, CFileRejectArrayTypeof) { "fail: Too complex to extract."); } -TEST_F(ExtractFunctionTest, CFileRejectVariableLengthArray) { - // A VLA's bound is an arbitrary expression -- typically another - // variable's name -- that isn't necessarily (and here isn't at all) - // itself a parameter of the extracted function, and even printing it - // via the usual array-decay path would reference a name meaningless - // in the new scope. Here the VLA bound isn't even at the top level - // (it's the inner dimension of a 2D array), exercising that - // `isVariablyModifiedType()` catches it regardless of nesting depth. +TEST_F(ExtractFunctionTest, CFileAllowSingleDimensionVariableLengthArray) { + // A VLA's own outermost dimension is never a problem: once decayed, + // it no longer appears in the parameter type at all (`int A[N]` + // decays to plain `int *A`, not something that needs to mention `N`). + FileName = "a.c"; + Context = File; + EXPECT_THAT(apply(R"cpp( + void foo(int N) { + int A[N]; + [[A[0] = 1;]] + })cpp"), + HasSubstr("extracted(int *A)")); +} + +TEST_F(ExtractFunctionTest, CFileRejectMultiDimensionVariableLengthArray) { + // Unlike the single-dimension case, an inner dimension that's still + // variable-length after decaying the outer one (`int A[2][N]` decays + // to `int (*A)[N]`) is a real hazard: `N` is an arbitrary expression, + // typically another variable's name, that isn't necessarily (and + // here isn't at all) itself a parameter of the extracted function, + // and even printing it would reference a name meaningless in the new + // scope. FileName = "a.c"; Context = File; EXPECT_EQ(apply(R"cpp( @@ -1249,6 +1263,21 @@ TEST_F(ExtractFunctionTest, CFileRejectVariableLengthArray) { "fail: Too complex to extract."); } +TEST_F(ExtractFunctionTest, CXXVariableLengthArrayByReference) { + // In C++ (where clang also supports VLAs as a GNU extension), this + // isn't a problem at all: an unmutated array always stays a + // reference there (see ConstParametersScalarsByValue's array case), + // never decaying to a pointer, so there's no separate parameter that + // needs to agree with the array's own bound. + Context = File; + EXPECT_THAT(apply(R"cpp( + void foo(int N) { + int A[2][N]; + [[A[0][0] = 1;]] + })cpp"), + HasSubstr("extracted(int (&A)[2][N])")); +} + } // namespace } // namespace clangd } // namespace clang _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
