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

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.


================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:466
 
   auto DeclsUnderCursor = locateDeclAt(AST, IdentifierToken->location());
   if (DeclsUnderCursor.empty())
----------------
kadircet wrote:
> `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?
I think the main problem is that `NamedDecl->getCanonicalDecl()` returns a `Decl*`, which we need to do a `dyn_cast`.


================
Comment at: clang-tools-extra/clangd/refactor/RenameBlacklist.cpp:14
+
+bool isRenameBlacklisted(const NamedDecl &RenameDecl) {
+  static const auto *StdSymbols = new llvm::DenseSet<llvm::StringRef>({
----------------
kadircet wrote:
> I don't think there's much value in having that in its own file, could you move it into the rename.cpp ?
I thought we will extend this in the near future, moved to rename.cpp now as it is pretty short.


================
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());
----------------
kadircet wrote:
> maybe `SCOPEDT_TRACE(T)` instead?
good point, reverted the change in this patch, and  will do this in a separate patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73450