kbobyrev created this revision. kbobyrev added a reviewer: hokein. Herald added subscribers: usaxena95, arphaman. kbobyrev requested review of this revision. Herald added subscribers: cfe-commits, MaskRay, ilya-biryukov. Herald added a project: clang-tools-extra.
This is a follow-up on D116643 <https://reviews.llvm.org/D116643>. `isInSystemHeader` check already detects symbols coming from the Standard Library, so searching for the qualified name in StdSymbolMap.inc is no longer necessary. The tests filtering out purely based on the symbol qualified names are removed. Repository: rG LLVM Github Monorepo https://reviews.llvm.org/D117491 Files: clang-tools-extra/clangd/refactor/Rename.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 @@ -880,21 +880,6 @@ void f(X x) {x+^+;})cpp", "no symbol", HeaderFile}, - {R"cpp(// disallow rename on excluded symbols (e.g. std symbols) - namespace std { - class str^ing {}; - } - )cpp", - "not a supported kind", !HeaderFile}, - {R"cpp(// disallow rename on excluded symbols (e.g. std symbols) - namespace std { - inline namespace __u { - class str^ing {}; - } - } - )cpp", - "not a supported kind", !HeaderFile}, - {R"cpp(// disallow rename on non-normal identifiers. @interface Foo {} -(int) fo^o:(int)x; // Token is an identifier, but declaration name isn't a simple identifier. @@ -1199,11 +1184,14 @@ } TEST(RenameTest, NoRenameOnSymbolsFromSystemHeaders) { - // filter out references not from main file. llvm::StringRef Test = R"cpp( + #include <cstdlib> #include <system> + SystemSym^bol abc; + + void foo() { at^oi("9000"); } )cpp"; Annotations Code(Test); @@ -1211,14 +1199,22 @@ TU.AdditionalFiles["system"] = R"cpp( class SystemSymbol {}; )cpp"; + TU.AdditionalFiles["cstdlib"] = R"cpp( + int atoi(const char *str); + )cpp"; TU.ExtraArgs = {"-isystem", testRoot()}; auto AST = TU.build(); llvm::StringRef NewName = "abcde"; - auto Results = rename({Code.point(), NewName, AST, testPath(TU.Filename)}); - EXPECT_FALSE(Results) << "expected rename returned an error: " << Code.code(); - auto ActualMessage = llvm::toString(Results.takeError()); - EXPECT_THAT(ActualMessage, testing::HasSubstr("not a supported kind")); + // Clangd will not allow renaming symbols from the system headers for + // correctness. + for (auto &Point : Code.points()) { + auto Results = rename({Point, NewName, AST, testPath(TU.Filename)}); + EXPECT_FALSE(Results) << "expected rename returned an error: " + << Code.code(); + auto ActualMessage = llvm::toString(Results.takeError()); + EXPECT_THAT(ActualMessage, testing::HasSubstr("not a supported kind")); + } } TEST(RenameTest, ProtobufSymbolIsExcluded) { Index: clang-tools-extra/clangd/refactor/Rename.cpp =================================================================== --- clang-tools-extra/clangd/refactor/Rename.cpp +++ clang-tools-extra/clangd/refactor/Rename.cpp @@ -164,18 +164,8 @@ // to be good candidates for modification. bool isExcluded(const NamedDecl &RenameDecl) { const auto &SM = RenameDecl.getASTContext().getSourceManager(); - if (SM.isInSystemHeader(RenameDecl.getLocation())) - return true; - if (isProtoFile(RenameDecl.getLocation(), SM)) - return true; - // FIXME: Remove this std symbol list, as they should be covered by the - // above isInSystemHeader check. - static const auto *StdSymbols = new llvm::DenseSet<llvm::StringRef>({ -#define SYMBOL(Name, NameSpace, Header) {#NameSpace #Name}, -#include "StdSymbolMap.inc" -#undef SYMBOL - }); - return StdSymbols->count(printQualifiedName(RenameDecl)); + return SM.isInSystemHeader(RenameDecl.getLocation()) || + isProtoFile(RenameDecl.getLocation(), SM); } enum class ReasonToReject {
Index: clang-tools-extra/clangd/unittests/RenameTests.cpp =================================================================== --- clang-tools-extra/clangd/unittests/RenameTests.cpp +++ clang-tools-extra/clangd/unittests/RenameTests.cpp @@ -880,21 +880,6 @@ void f(X x) {x+^+;})cpp", "no symbol", HeaderFile}, - {R"cpp(// disallow rename on excluded symbols (e.g. std symbols) - namespace std { - class str^ing {}; - } - )cpp", - "not a supported kind", !HeaderFile}, - {R"cpp(// disallow rename on excluded symbols (e.g. std symbols) - namespace std { - inline namespace __u { - class str^ing {}; - } - } - )cpp", - "not a supported kind", !HeaderFile}, - {R"cpp(// disallow rename on non-normal identifiers. @interface Foo {} -(int) fo^o:(int)x; // Token is an identifier, but declaration name isn't a simple identifier. @@ -1199,11 +1184,14 @@ } TEST(RenameTest, NoRenameOnSymbolsFromSystemHeaders) { - // filter out references not from main file. llvm::StringRef Test = R"cpp( + #include <cstdlib> #include <system> + SystemSym^bol abc; + + void foo() { at^oi("9000"); } )cpp"; Annotations Code(Test); @@ -1211,14 +1199,22 @@ TU.AdditionalFiles["system"] = R"cpp( class SystemSymbol {}; )cpp"; + TU.AdditionalFiles["cstdlib"] = R"cpp( + int atoi(const char *str); + )cpp"; TU.ExtraArgs = {"-isystem", testRoot()}; auto AST = TU.build(); llvm::StringRef NewName = "abcde"; - auto Results = rename({Code.point(), NewName, AST, testPath(TU.Filename)}); - EXPECT_FALSE(Results) << "expected rename returned an error: " << Code.code(); - auto ActualMessage = llvm::toString(Results.takeError()); - EXPECT_THAT(ActualMessage, testing::HasSubstr("not a supported kind")); + // Clangd will not allow renaming symbols from the system headers for + // correctness. + for (auto &Point : Code.points()) { + auto Results = rename({Point, NewName, AST, testPath(TU.Filename)}); + EXPECT_FALSE(Results) << "expected rename returned an error: " + << Code.code(); + auto ActualMessage = llvm::toString(Results.takeError()); + EXPECT_THAT(ActualMessage, testing::HasSubstr("not a supported kind")); + } } TEST(RenameTest, ProtobufSymbolIsExcluded) { Index: clang-tools-extra/clangd/refactor/Rename.cpp =================================================================== --- clang-tools-extra/clangd/refactor/Rename.cpp +++ clang-tools-extra/clangd/refactor/Rename.cpp @@ -164,18 +164,8 @@ // to be good candidates for modification. bool isExcluded(const NamedDecl &RenameDecl) { const auto &SM = RenameDecl.getASTContext().getSourceManager(); - if (SM.isInSystemHeader(RenameDecl.getLocation())) - return true; - if (isProtoFile(RenameDecl.getLocation(), SM)) - return true; - // FIXME: Remove this std symbol list, as they should be covered by the - // above isInSystemHeader check. - static const auto *StdSymbols = new llvm::DenseSet<llvm::StringRef>({ -#define SYMBOL(Name, NameSpace, Header) {#NameSpace #Name}, -#include "StdSymbolMap.inc" -#undef SYMBOL - }); - return StdSymbols->count(printQualifiedName(RenameDecl)); + return SM.isInSystemHeader(RenameDecl.getLocation()) || + isProtoFile(RenameDecl.getLocation(), SM); } enum class ReasonToReject {
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits