[PATCH] D73450: [clangd] Add a symbol-name-based blacklist for rename.

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.


================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:138
     IsMainFileOnly = false;
-  bool IsIndexable =
-      isa<NamedDecl>(RenameDecl) &&
-      SymbolCollector::shouldCollectSymbol(
-          cast<NamedDecl>(RenameDecl), RenameDecl.getASTContext(),
-          SymbolCollector::Options(), IsMainFileOnly);
+  bool IsIndexable = SymbolCollector::shouldCollectSymbol(
+      RenameDecl, RenameDecl.getASTContext(), SymbolCollector::Options(),
----------------
nit: maybe just `if (!shouldCollectSymbol...) return nonindexable;` ?


================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:466
 
   auto DeclsUnderCursor = locateDeclAt(AST, IdentifierToken->location());
   if (DeclsUnderCursor.empty())
----------------
`locateDeclAt` is already working on `NamedDecl`s but returning a set of `Decl`s could you rather update that helper to return a set of `NamedDecl`s instead?


================
Comment at: clang-tools-extra/clangd/refactor/RenameBlacklist.cpp:14
+
+bool isRenameBlacklisted(const NamedDecl &RenameDecl) {
+  static const auto *StdSymbols = new llvm::DenseSet<llvm::StringRef>({
----------------
I don't think there's much value in having that in its own file, could you move it into the rename.cpp ?


================
Comment at: clang-tools-extra/clangd/unittests/RenameTests.cpp:453
           rename({RenamePos, NewName, AST, testPath(TU.Filename)});
-      ASSERT_TRUE(bool(RenameResult)) << RenameResult.takeError();
+      ASSERT_TRUE(bool(RenameResult)) << RenameResult.takeError() << T;
       ASSERT_EQ(1u, RenameResult->size());
----------------
maybe `SCOPEDT_TRACE(T)` instead?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D73450/new/

https://reviews.llvm.org/D73450