llvmorg-github-actions[bot] wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang-tools-extra Author: Christian Kandeler (ckandeler) <details> <summary>Changes</summary> Every captured variable was always passed by non-const reference, even when the extracted code never modifies it, resulting in misleading function signatures. Use clang's `ExprMutationAnalyzer` (already used by several clang-tidy checks for the same purpose, e.g. `misc-const-correctness`) to check whether each captured variable is ever mutated within the extraction zone, and add `const` to the parameter's type when it isn't. Still passed by reference either way, to avoid a copy. Add a new `clangAnalysis` dependency to clangd's tweaks library for `ExprMutationAnalyzer`. Assisted-by: Claude --- Full diff: https://github.com/llvm/llvm-project/pull/225666.diff 5 Files Affected: - (modified) clang-tools-extra/clangd/refactor/tweaks/CMakeLists.txt (+1) - (modified) clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp (+23-7) - (modified) clang-tools-extra/clangd/tool/CMakeLists.txt (+1) - (modified) clang-tools-extra/clangd/unittests/CMakeLists.txt (+1) - (modified) clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp (+59-3) ``````````diff diff --git a/clang-tools-extra/clangd/refactor/tweaks/CMakeLists.txt b/clang-tools-extra/clangd/refactor/tweaks/CMakeLists.txt index 1d6e38088ad67a..f14013b0270a72 100644 --- a/clang-tools-extra/clangd/refactor/tweaks/CMakeLists.txt +++ b/clang-tools-extra/clangd/refactor/tweaks/CMakeLists.txt @@ -34,6 +34,7 @@ add_clang_library(clangDaemonTweaks OBJECT SwapIfBranches.cpp LINK_LIBS + clangAnalysis clangAST clangBasic clangDaemon diff --git a/clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp b/clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp index eb7a9faa65a81f..0603d5d5a5b651 100644 --- a/clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp +++ b/clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp @@ -60,6 +60,7 @@ #include "clang/AST/NestedNameSpecifier.h" #include "clang/AST/RecursiveASTVisitor.h" #include "clang/AST/Stmt.h" +#include "clang/Analysis/Analyses/ExprMutationAnalyzer.h" #include "clang/Basic/LangOptions.h" #include "clang/Basic/SourceLocation.h" #include "clang/Basic/SourceManager.h" @@ -737,12 +738,27 @@ CapturedZoneInfo captureZoneInfo(const ExtractionZone &ExtZone) { return Result; } +// Whether VD is mutated anywhere within the extraction zone. If not, the +// corresponding parameter can safely be made const, even though it's still +// passed by reference. +// FIXME: Pass non-mutated parameters of built-in type by value. +bool isCapturedDeclMutated(const ValueDecl *VD, const ExtractionZone &ExtZone) { + ASTContext &Context = ExtZone.EnclosingFunction->getASTContext(); + for (const Stmt *RootStmt : ExtZone.RootStmts) { + ExprMutationAnalyzer Analyzer(*RootStmt, Context); + if (Analyzer.isMutated(VD)) + return true; + } + return false; +} + // Adds parameters to ExtractedFunc. // Returns true if able to find the parameters successfully and no hoisting // needed. // FIXME: Check if the declaration has a local/anonymous type bool createParameters(NewFunction &ExtractedFunc, - const CapturedZoneInfo &CapturedInfo) { + const CapturedZoneInfo &CapturedInfo, + const ExtractionZone &ExtZone) { for (const auto &KeyVal : CapturedInfo.DeclInfoMap) { const auto &DeclInfo = KeyVal.second; // If a Decl was Declared in zone and referenced in post zone, it @@ -764,11 +780,11 @@ bool createParameters(NewFunction &ExtractedFunc, return false; // Parameter qualifiers are same as the Decl's qualifiers. QualType TypeInfo = VD->getType().getNonReferenceType(); - // FIXME: Need better qualifier checks: check mutated status for - // Decl(e.g. was it assigned, passed as nonconst argument, etc) - // FIXME: check if parameter will be a non l-value reference. - // FIXME: We don't want to always pass variables of types like int, - // pointers, etc by reference. + // Add const if it's not mutated in the zone: it's still passed by + // reference to avoid a copy, but the reference doesn't need to be + // mutable. + if (!isCapturedDeclMutated(VD, ExtZone)) + TypeInfo.addConst(); bool IsPassedByReference = true; // We use the index of declaration as the ordering priority for parameters. ExtractedFunc.Parameters.push_back({std::string(VD->getName()), TypeInfo, @@ -868,7 +884,7 @@ llvm::Expected<NewFunction> getExtractedFunction(ExtractionZone &ExtZone, ExtractedFunc.DefinitionPoint = ExtZone.getInsertionPoint(); ExtractedFunc.CallerReturnsValue = CapturedInfo.AlwaysReturns; - if (!createParameters(ExtractedFunc, CapturedInfo) || + if (!createParameters(ExtractedFunc, CapturedInfo, ExtZone) || !generateReturnProperties(ExtractedFunc, *ExtZone.EnclosingFunction, CapturedInfo)) return error("Too complex to extract."); diff --git a/clang-tools-extra/clangd/tool/CMakeLists.txt b/clang-tools-extra/clangd/tool/CMakeLists.txt index 1bd6a1f864dd3b..99f625b0ad7340 100644 --- a/clang-tools-extra/clangd/tool/CMakeLists.txt +++ b/clang-tools-extra/clangd/tool/CMakeLists.txt @@ -46,6 +46,7 @@ target_link_libraries(clangdMain clang_target_link_libraries(clangd PRIVATE + clangAnalysis clangAST clangBasic clangLex diff --git a/clang-tools-extra/clangd/unittests/CMakeLists.txt b/clang-tools-extra/clangd/unittests/CMakeLists.txt index d596ba77efd4ae..4a562b1b5f4cd7 100644 --- a/clang-tools-extra/clangd/unittests/CMakeLists.txt +++ b/clang-tools-extra/clangd/unittests/CMakeLists.txt @@ -160,6 +160,7 @@ target_include_directories(ClangdTests PUBLIC clang_target_link_libraries(ClangdTests PRIVATE + clangAnalysis clangAST clangASTMatchers clangBasic diff --git a/clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp b/clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp index c55fc17ae83e9a..4381eaf985f3da 100644 --- a/clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp +++ b/clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp @@ -61,7 +61,9 @@ TEST_F(ExtractFunctionTest, FunctionTest) { } TEST_F(ExtractFunctionTest, FileTest) { - // Check all parameters are in order + // Check all parameters are in order. `a` and `ptr` are mutated in the + // zone (`+=` and postfix `++` respectively), so stay non-const; `b` and + // `foo` are only read, so become const references. std::string ParameterCheckInput = R"cpp( struct Foo { int x; @@ -77,7 +79,7 @@ void f(int a) { struct Foo { int x; }; -void extracted(int &a, int &b, int * &ptr, Foo &foo) { +void extracted(int &a, const int &b, int * &ptr, const Foo &foo) { a += foo.x + b; *ptr++; } @@ -574,7 +576,7 @@ TEST_F(ExtractFunctionTest, ExistingReturnStatement) { // FIXME: avoid emitting redundant braces const char *After = R"cpp( bool lucky(int N); - int extracted(int &Min, int &Max) { + int extracted(const int &Min, const int &Max) { { for (int I = Min; I <= Max; ++I) if (lucky(I)) @@ -777,6 +779,60 @@ TEST_F(ExtractFunctionTest, VarDeclInitializer) { HasSubstr("extracted")); } +TEST_F(ExtractFunctionTest, ConstParameters) { + Context = File; + // A captured variable that's only read becomes a const reference. + EXPECT_THAT(apply(R"cpp( + void use(int); + void f(int x) { [[use(x);]] } + )cpp"), + HasSubstr("void extracted(const int &x)")); + // Direct assignment: stays non-const. + EXPECT_THAT(apply("void f(int x) { [[x = 1;]] }"), + HasSubstr("void extracted(int &x)")); + // Compound assignment: stays non-const. + EXPECT_THAT(apply("void f(int x) { [[x += 1;]] }"), + HasSubstr("void extracted(int &x)")); + // Increment/decrement: stays non-const. + EXPECT_THAT(apply("void f(int x) { [[++x;]] }"), + HasSubstr("void extracted(int &x)")); + // A non-const method call may mutate the object: stays non-const. + EXPECT_THAT(apply(R"cpp( + struct S { void mutate(); }; + void f(S s) { [[s.mutate();]] } + )cpp"), + HasSubstr("void extracted(S &s)")); + // A const method call cannot mutate the object: becomes const. + EXPECT_THAT(apply(R"cpp( + struct S { void inspect() const; }; + void f(S s) { [[s.inspect();]] } + )cpp"), + HasSubstr("void extracted(const S &s)")); + // Passed to a parameter taking a non-const reference: stays non-const, + // since the callee could mutate it through that reference. + EXPECT_THAT(apply(R"cpp( + void mayMutate(int &); + void f(int x) { [[mayMutate(x);]] } + )cpp"), + HasSubstr("void extracted(int &x)")); + // Passed to a parameter taking a const reference or by value: becomes + // const, since neither can mutate the caller's variable. + EXPECT_THAT(apply(R"cpp( + void readOnly(const int &); + void f(int x) { [[readOnly(x);]] } + )cpp"), + HasSubstr("void extracted(const int &x)")); + EXPECT_THAT(apply(R"cpp( + void byValue(int); + void f(int x) { [[byValue(x);]] } + )cpp"), + HasSubstr("void extracted(const int &x)")); + // A parameter that's already declared const stays as-is (no double + // const). + EXPECT_THAT(apply("void use(int); void f(const int x) { [[use(x);]] }"), + HasSubstr("void extracted(const int &x)")); +} + } // namespace } // namespace clangd } // namespace clang `````````` </details> https://github.com/llvm/llvm-project/pull/225666 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
