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