hokein created this revision.
hokein added a reviewer: sammccall.
Herald added subscribers: usaxena95, kadircet, arphaman, jkorous, MaskRay, 
ilya-biryukov.
Herald added a project: clang.

Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D74834

Files:
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/clangd/ClangdServer.h
  clang-tools-extra/clangd/refactor/Rename.cpp
  clang-tools-extra/clangd/refactor/Rename.h
  clang-tools-extra/clangd/tool/ClangdMain.cpp
  clang-tools-extra/clangd/unittests/RenameTests.cpp

Index: clang-tools-extra/clangd/unittests/RenameTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/RenameTests.cpp
+++ clang-tools-extra/clangd/unittests/RenameTests.cpp
@@ -714,8 +714,13 @@
   TestTU TU = TestTU::withCode(MainCode.code());
   auto AST = TU.build();
   llvm::StringRef NewName = "newName";
-  auto Results = rename({MainCode.point(), NewName, AST, MainFilePath,
-                         Index.get(), /*CrossFile=*/true, GetDirtyBuffer});
+  auto Results = rename({MainCode.point(),
+                         NewName,
+                         AST,
+                         MainFilePath,
+                         Index.get(),
+                         {/*CrossFile=*/true},
+                         GetDirtyBuffer});
   ASSERT_TRUE(bool(Results)) << Results.takeError();
   EXPECT_THAT(
       applyEdits(std::move(*Results)),
@@ -730,8 +735,13 @@
   // Set a file "bar.cc" on disk.
   TU.AdditionalFiles["bar.cc"] = std::string(BarCode.code());
   AST = TU.build();
-  Results = rename({MainCode.point(), NewName, AST, MainFilePath, Index.get(),
-                    /*CrossFile=*/true, GetDirtyBuffer});
+  Results = rename({MainCode.point(),
+                    NewName,
+                    AST,
+                    MainFilePath,
+                    Index.get(),
+                    {/*CrossFile=*/true},
+                    GetDirtyBuffer});
   ASSERT_TRUE(bool(Results)) << Results.takeError();
   EXPECT_THAT(
       applyEdits(std::move(*Results)),
@@ -761,8 +771,13 @@
                        Callback) const override {}
     size_t estimateMemoryUsage() const override { return 0; }
   } PIndex;
-  Results = rename({MainCode.point(), NewName, AST, MainFilePath, &PIndex,
-                    /*CrossFile=*/true, GetDirtyBuffer});
+  Results = rename({MainCode.point(),
+                    NewName,
+                    AST,
+                    MainFilePath,
+                    &PIndex,
+                    {/*CrossFile=*/true},
+                    GetDirtyBuffer});
   EXPECT_FALSE(Results);
   EXPECT_THAT(llvm::toString(Results.takeError()),
               testing::HasSubstr("too many occurrences"));
@@ -803,9 +818,12 @@
 
     RefsRequest *Out;
   } RIndex(&Req);
-  auto Results =
-      rename({MainCode.point(), "NewName", AST, testPath("main.cc"), &RIndex,
-              /*CrossFile=*/true});
+  auto Results = rename({MainCode.point(),
+                         "NewName",
+                         AST,
+                         testPath("main.cc"),
+                         &RIndex,
+                         {/*CrossFile=*/true}});
   ASSERT_TRUE(bool(Results)) << Results.takeError();
   const auto HeaderSymbols = TU.headerSymbols();
   EXPECT_THAT(Req.IDs,
@@ -850,8 +868,9 @@
     Ref ReturnedRef;
   } DIndex(XRefInBarCC);
   llvm::StringRef NewName = "newName";
-  auto Results = rename({MainCode.point(), NewName, AST, MainFilePath, &DIndex,
-                         /*CrossFile=*/true});
+  RenameOptions RenameOpts{true, 50};
+  auto Results = rename(
+      {MainCode.point(), NewName, AST, MainFilePath, &DIndex, RenameOpts});
   ASSERT_TRUE(bool(Results)) << Results.takeError();
   EXPECT_THAT(
       applyEdits(std::move(*Results)),
@@ -1021,7 +1040,7 @@
     FS.Files[FooCCPath] = std::string(FooCC.code());
 
     auto ServerOpts = ClangdServer::optsForTest();
-    ServerOpts.CrossFileRename = true;
+    ServerOpts.RenameOpts.AllowCrossFile = true;
     ServerOpts.BuildDynamicSymbolIndex = true;
     ClangdServer Server(CDB, FS, ServerOpts);
 
Index: clang-tools-extra/clangd/tool/ClangdMain.cpp
===================================================================
--- clang-tools-extra/clangd/tool/ClangdMain.cpp
+++ clang-tools-extra/clangd/tool/ClangdMain.cpp
@@ -628,7 +628,7 @@
   }
   Opts.StaticIndex = StaticIdx.get();
   Opts.AsyncThreadsCount = WorkerThreadsCount;
-  Opts.CrossFileRename = CrossFileRename;
+  Opts.RenameOpts.AllowCrossFile = CrossFileRename;
 
   clangd::CodeCompleteOptions CCOpts;
   CCOpts.IncludeIneligibleResults = IncludeIneligibleResults;
Index: clang-tools-extra/clangd/refactor/Rename.h
===================================================================
--- clang-tools-extra/clangd/refactor/Rename.h
+++ clang-tools-extra/clangd/refactor/Rename.h
@@ -26,6 +26,14 @@
 using DirtyBufferGetter =
     llvm::function_ref<llvm::Optional<std::string>(PathRef AbsPath)>;
 
+struct RenameOptions {
+  bool AllowCrossFile = false;
+  /// The mamimum number of affected files (0 means no limit), only meaningful
+  /// when AllowCrossFile = true.
+  /// If the actual number exceeds the limit, rename is forbidden.
+  size_t LimitFiles = 50;
+};
+
 struct RenameInputs {
   Position Pos; // the position triggering the rename
   llvm::StringRef NewName;
@@ -35,7 +43,7 @@
 
   const SymbolIndex *Index = nullptr;
 
-  bool AllowCrossFile = false;
+  RenameOptions Opts = {};
   // When set, used by the rename to get file content for all rename-related
   // files.
   // If there is no corresponding dirty buffer, we will use the file content
Index: clang-tools-extra/clangd/refactor/Rename.cpp
===================================================================
--- clang-tools-extra/clangd/refactor/Rename.cpp
+++ clang-tools-extra/clangd/refactor/Rename.cpp
@@ -322,7 +322,8 @@
 // grouped by the absolute file path.
 llvm::Expected<llvm::StringMap<std::vector<Range>>>
 findOccurrencesOutsideFile(const NamedDecl &RenameDecl,
-                           llvm::StringRef MainFile, const SymbolIndex &Index) {
+                           llvm::StringRef MainFile, const SymbolIndex &Index,
+                           size_t MaxLimitFiles) {
   trace::Span Tracer("FindOccurrencesOutsideFile");
   RefsRequest RQuest;
   RQuest.IDs.insert(*getSymbolID(&RenameDecl));
@@ -337,8 +338,6 @@
 
   // Absolute file path => rename occurrences in that file.
   llvm::StringMap<std::vector<Range>> AffectedFiles;
-  // FIXME: Make the limit customizable.
-  static constexpr size_t MaxLimitFiles = 50;
   bool HasMore = Index.refs(RQuest, [&](const Ref &R) {
     if (AffectedFiles.size() > MaxLimitFiles)
       return;
@@ -387,11 +386,11 @@
 // there is no dirty buffer.
 llvm::Expected<FileEdits> renameOutsideFile(
     const NamedDecl &RenameDecl, llvm::StringRef MainFilePath,
-    llvm::StringRef NewName, const SymbolIndex &Index,
+    llvm::StringRef NewName, const SymbolIndex &Index, size_t MaxLimitFiles,
     llvm::function_ref<llvm::Expected<std::string>(PathRef)> GetFileContent) {
   trace::Span Tracer("RenameOutsideFile");
-  auto AffectedFiles =
-      findOccurrencesOutsideFile(RenameDecl, MainFilePath, Index);
+  auto AffectedFiles = findOccurrencesOutsideFile(RenameDecl, MainFilePath,
+                                                  Index, MaxLimitFiles);
   if (!AffectedFiles)
     return AffectedFiles.takeError();
   FileEdits Results;
@@ -473,6 +472,7 @@
 
 llvm::Expected<FileEdits> rename(const RenameInputs &RInputs) {
   trace::Span Tracer("Rename flow");
+  const auto &Opts = RInputs.Opts;
   ParsedAST &AST = RInputs.AST;
   const SourceManager &SM = AST.getSourceManager();
   llvm::StringRef MainFileCode = SM.getBufferData(SM.getMainFileID());
@@ -520,7 +520,7 @@
   const auto &RenameDecl =
       llvm::cast<NamedDecl>(*(*DeclsUnderCursor.begin())->getCanonicalDecl());
   auto Reject = renameable(RenameDecl, RInputs.MainFilePath, RInputs.Index,
-                           RInputs.AllowCrossFile);
+                           Opts.AllowCrossFile);
   if (Reject)
     return makeError(*Reject);
 
@@ -537,7 +537,7 @@
   if (!MainFileRenameEdit)
     return MainFileRenameEdit.takeError();
 
-  if (!RInputs.AllowCrossFile) {
+  if (!Opts.AllowCrossFile) {
     // Within-file rename: just return the main file results.
     return FileEdits(
         {std::make_pair(RInputs.MainFilePath,
@@ -548,9 +548,11 @@
   // Renameable safely guards us that at this point we are renaming a local
   // symbol if we don't have index.
   if (RInputs.Index) {
-    auto OtherFilesEdits =
-        renameOutsideFile(RenameDecl, RInputs.MainFilePath, RInputs.NewName,
-                          *RInputs.Index, GetFileContent);
+    auto OtherFilesEdits = renameOutsideFile(
+        RenameDecl, RInputs.MainFilePath, RInputs.NewName, *RInputs.Index,
+        Opts.LimitFiles == 0 ? std::numeric_limits<size_t>::max()
+                             : Opts.LimitFiles - 1,
+        GetFileContent);
     if (!OtherFilesEdits)
       return OtherFilesEdits.takeError();
     Results = std::move(*OtherFilesEdits);
Index: clang-tools-extra/clangd/ClangdServer.h
===================================================================
--- clang-tools-extra/clangd/ClangdServer.h
+++ clang-tools-extra/clangd/ClangdServer.h
@@ -142,8 +142,8 @@
     /// Enable semantic highlighting features.
     bool SemanticHighlighting = false;
 
-    /// Enable cross-file rename feature.
-    bool CrossFileRename = false;
+    /// Options for rename.
+    RenameOptions RenameOpts;
 
     /// Returns true if the tweak should be enabled.
     std::function<bool(const Tweak &)> TweakFilter = [](const Tweak &T) {
@@ -340,7 +340,7 @@
   // can be caused by missing includes (e.g. member access in incomplete type).
   bool SuggestMissingIncludes = false;
 
-  bool CrossFileRename = false;
+  RenameOptions RenameOpts;
 
   std::function<bool(const Tweak &)> TweakFilter;
 
Index: clang-tools-extra/clangd/ClangdServer.cpp
===================================================================
--- clang-tools-extra/clangd/ClangdServer.cpp
+++ clang-tools-extra/clangd/ClangdServer.cpp
@@ -131,7 +131,7 @@
                      : nullptr),
       GetClangTidyOptions(Opts.GetClangTidyOptions),
       SuggestMissingIncludes(Opts.SuggestMissingIncludes),
-      CrossFileRename(Opts.CrossFileRename), TweakFilter(Opts.TweakFilter),
+      RenameOpts(Opts.RenameOpts), TweakFilter(Opts.TweakFilter),
       WorkspaceRoot(Opts.WorkspaceRoot),
       // Pass a callback into `WorkScheduler` to extract symbols from a newly
       // parsed file and rebuild the file index synchronously each time an AST
@@ -338,14 +338,13 @@
         SM, CharSourceRange::getCharRange(TouchingIdentifier->location(),
                                           TouchingIdentifier->endLocation()));
 
-    if (CrossFileRename)
+    if (RenameOpts.AllowCrossFile)
       // FIXME: we now assume cross-file rename always succeeds, revisit this.
       return CB(Range);
 
     // Performing the local rename isn't substantially more expensive than
     // doing an AST-based check, so we just rename and throw away the results.
-    auto Changes = clangd::rename({Pos, "dummy", AST, File, Index,
-                                   /*AllowCrossFile=*/false,
+    auto Changes = clangd::rename({Pos, "dummy", AST, File, Index, RenameOpts,
                                    /*GetDirtyBuffer=*/nullptr});
     if (!Changes) {
       // LSP says to return null on failure, but that will result in a generic
@@ -374,8 +373,8 @@
         return llvm::None;
       return It->second;
     };
-    auto Edits = clangd::rename({Pos, NewName, InpAST->AST, File, Index,
-                                 CrossFileRename, GetDirtyBuffer});
+    auto Edits = clangd::rename(
+        {Pos, NewName, InpAST->AST, File, Index, RenameOpts, GetDirtyBuffer});
     if (!Edits)
       return CB(Edits.takeError());
 
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to