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