hokein created this revision.
hokein added a reviewer: sammccall.
Herald added subscribers: usaxena95, kadircet, arphaman.
hokein requested review of this revision.
Herald added subscribers: MaskRay, ilya-biryukov.
Herald added a project: clang.
and simplify the code.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D96578
Files:
clang-tools-extra/clangd/ClangdServer.cpp
clang-tools-extra/clangd/refactor/Rename.cpp
clang-tools-extra/clangd/refactor/Rename.h
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
@@ -189,25 +189,6 @@
};
)cpp",
- // Class methods overrides.
- R"cpp(
- struct A {
- virtual void [[f^oo]]() {}
- };
- struct B : A {
- void [[f^oo]]() override {}
- };
- struct C : B {
- void [[f^oo]]() override {}
- };
-
- void func() {
- A().[[f^oo]]();
- B().[[f^oo]]();
- C().[[f^oo]]();
- }
- )cpp",
-
// Templated method instantiation.
R"cpp(
template<typename T>
@@ -831,13 +812,10 @@
auto TU = TestTU::withCode(Code.code());
TU.ExtraArgs.push_back("-xobjective-c++");
auto AST = TU.build();
+ auto Index = TU.index();
for (const auto &RenamePos : Code.points()) {
- auto RenameResult = rename({RenamePos,
- NewName,
- AST,
- testPath(TU.Filename),
- /*Index*/ nullptr,
- {/*CrossFile*/ false}});
+ auto RenameResult =
+ rename({RenamePos, NewName, AST, testPath(TU.Filename), Index.get()});
ASSERT_TRUE(bool(RenameResult)) << RenameResult.takeError();
ASSERT_EQ(1u, RenameResult->GlobalChanges.size());
EXPECT_EQ(
@@ -851,95 +829,40 @@
struct Case {
const char *Code;
const char* ErrorMessage; // null if no error
- bool IsHeaderFile;
- const SymbolIndex *Index;
llvm::StringRef NewName = "DummyName";
};
- TestTU OtherFile = TestTU::withCode("Outside s; auto ss = &foo;");
- const char *CommonHeader = R"cpp(
- class Outside {};
- void foo();
- )cpp";
- OtherFile.HeaderCode = CommonHeader;
- OtherFile.Filename = "other.cc";
- // The index has a "Outside" reference and a "foo" reference.
- auto OtherFileIndex = OtherFile.index();
- const SymbolIndex *Index = OtherFileIndex.get();
-
- const bool HeaderFile = true;
Case Cases[] = {
{R"cpp(// allow -- function-local
void f(int [[Lo^cal]]) {
[[Local]] = 2;
}
)cpp",
- nullptr, HeaderFile, Index},
-
- {R"cpp(// allow -- symbol is indexable and has no refs in index.
- void [[On^lyInThisFile]]();
- )cpp",
- nullptr, HeaderFile, Index},
-
- {R"cpp(
- void ^f();
- )cpp",
- "keyword", HeaderFile, Index, "return"},
-
- {R"cpp(// disallow -- symbol is indexable and has other refs in index.
- void f() {
- Out^side s;
- }
- )cpp",
- "used outside main file", HeaderFile, Index},
-
- {R"cpp(// disallow -- symbol in anonymous namespace in header is not indexable.
- namespace {
- class Unin^dexable {};
- }
- )cpp",
- "not eligible for indexing", HeaderFile, Index},
-
- {R"cpp(// allow -- symbol in anonymous namespace in non-header file is indexable.
- namespace {
- class [[F^oo]] {};
- }
- )cpp",
- nullptr, !HeaderFile, Index},
+ nullptr},
{R"cpp(// disallow -- namespace symbol isn't supported
namespace n^s {}
)cpp",
- "not a supported kind", HeaderFile, Index},
+ "not a supported kind"},
{
R"cpp(
#define MACRO 1
int s = MAC^RO;
)cpp",
- "not a supported kind", HeaderFile, Index},
+ "not a supported kind"},
{
R"cpp(
struct X { X operator++(int); };
void f(X x) {x+^+;})cpp",
- "no symbol", HeaderFile, Index},
-
- {R"cpp(// foo is declared outside the file.
- void fo^o() {}
- )cpp",
- "used outside main file", !HeaderFile /*cc file*/, Index},
-
- {R"cpp(
- // We should detect the symbol is used outside the file from the AST.
- void fo^o() {})cpp",
- "used outside main file", !HeaderFile, nullptr /*no index*/},
+ "no symbol"},
{R"cpp(// disallow rename on excluded symbols (e.g. std symbols)
namespace std {
class str^ing {};
}
)cpp",
- "not a supported kind", !HeaderFile, Index},
+ "not a supported kind"},
{R"cpp(// disallow rename on excluded symbols (e.g. std symbols)
namespace std {
inline namespace __u {
@@ -947,33 +870,38 @@
}
}
)cpp",
- "not a supported kind", !HeaderFile, Index},
+ "not a supported kind"},
{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.
@end
)cpp",
- "not a supported kind", HeaderFile, Index},
-
+ "not a supported kind"},
+ {R"cpp(// FIXME: rename virtual/override methods is not supported yet.
+ struct A {
+ virtual void f^oo() {}
+ };
+ )cpp",
+ "not a supported kind"},
{R"cpp(
void foo(int);
void foo(char);
template <typename T> void f(T t) {
fo^o(t);
})cpp",
- "multiple symbols", !HeaderFile, nullptr /*no index*/},
+ "multiple symbols"},
{R"cpp(// disallow rename on unrelated token.
cl^ass Foo {};
)cpp",
- "no symbol", !HeaderFile, nullptr},
+ "no symbol"},
{R"cpp(// disallow rename on unrelated token.
temp^late<typename T>
class Foo {};
)cpp",
- "no symbol", !HeaderFile, nullptr},
+ "no symbol"},
{R"cpp(
namespace {
@@ -981,13 +909,13 @@
int Va^r;
}
)cpp",
- "conflict", !HeaderFile, nullptr, "Conflict"},
+ "conflict", "Conflict"},
{R"cpp(
int Conflict;
int Va^r;
)cpp",
- "conflict", !HeaderFile, nullptr, "Conflict"},
+ "conflict", "Conflict"},
{R"cpp(
class Foo {
@@ -995,7 +923,7 @@
int Va^r;
};
)cpp",
- "conflict", !HeaderFile, nullptr, "Conflict"},
+ "conflict", "Conflict"},
{R"cpp(
enum E {
@@ -1003,7 +931,7 @@
Fo^o,
};
)cpp",
- "conflict", !HeaderFile, nullptr, "Conflict"},
+ "conflict", "Conflict"},
{R"cpp(
int Conflict;
@@ -1011,7 +939,7 @@
F^oo,
};
)cpp",
- "conflict", !HeaderFile, nullptr, "Conflict"},
+ "conflict", "Conflict"},
{R"cpp(
void func() {
@@ -1020,7 +948,7 @@
char Conflict;
}
)cpp",
- "conflict", !HeaderFile, nullptr, "Conflict"},
+ "conflict", "Conflict"},
{R"cpp(
void func() {
@@ -1029,7 +957,7 @@
}
}
)cpp",
- "conflict", !HeaderFile, nullptr, "Conflict"},
+ "conflict", "Conflict"},
{R"cpp(
void func() {
@@ -1039,7 +967,7 @@
}
}
)cpp",
- "conflict", !HeaderFile, nullptr, "Conflict"},
+ "conflict", "Conflict"},
{R"cpp(
void func() {
@@ -1049,7 +977,7 @@
}
}
)cpp",
- "conflict", !HeaderFile, nullptr, "Conflict"},
+ "conflict", "Conflict"},
{R"cpp(
void func() {
@@ -1058,7 +986,7 @@
}
}
)cpp",
- "conflict", !HeaderFile, nullptr, "Conflict"},
+ "conflict", "Conflict"},
{R"cpp(
void func() {
@@ -1068,7 +996,7 @@
}
}
)cpp",
- "conflict", !HeaderFile, nullptr, "Conflict"},
+ "conflict", "Conflict"},
{R"cpp(
void func() {
@@ -1076,14 +1004,14 @@
}
}
)cpp",
- "conflict", !HeaderFile, nullptr, "Conflict"},
+ "conflict", "Conflict"},
{R"cpp(
void func(int Conflict) {
bool V^ar;
}
)cpp",
- "conflict", !HeaderFile, nullptr, "Conflict"},
+ "conflict", "Conflict"},
{R"cpp(
void func(int Var);
@@ -1092,7 +1020,7 @@
bool Conflict;
}
)cpp",
- "conflict", !HeaderFile, nullptr, "Conflict"},
+ "conflict", "Conflict"},
{R"cpp(// No conflict: only forward declaration's argument is renamed.
void func(int [[V^ar]]);
@@ -1101,48 +1029,36 @@
bool Conflict;
}
)cpp",
- nullptr, !HeaderFile, nullptr, "Conflict"},
+ nullptr, "Conflict"},
{R"cpp(
void func(int V^ar, int Conflict) {
}
)cpp",
- "conflict", !HeaderFile, nullptr, "Conflict"},
+ "conflict", "Conflict"},
{R"cpp(// Trying to rename into the same name, SameName == SameName.
void func() {
int S^ameName;
}
)cpp",
- "new name is the same", !HeaderFile, nullptr, "SameName"},
+ "new name is the same", "SameName"},
{R"cpp(// Ensure it doesn't associate base specifier with base name.
struct A {};
struct B : priv^ate A {};
)cpp",
- "Cannot rename symbol: there is no symbol at the given location", false,
- nullptr},
+ "Cannot rename symbol: there is no symbol at the given location"},
};
for (const auto& Case : Cases) {
SCOPED_TRACE(Case.Code);
Annotations T(Case.Code);
TestTU TU = TestTU::withCode(T.code());
- TU.HeaderCode = CommonHeader;
TU.ExtraArgs.push_back("-fno-delayed-template-parsing");
- if (Case.IsHeaderFile) {
- // We open the .h file as the main file.
- TU.Filename = "test.h";
- // Parsing the .h file as C++ include.
- TU.ExtraArgs.push_back("-xobjective-c++-header");
- }
+ TU.ExtraArgs.push_back("-xobjective-c++");
auto AST = TU.build();
llvm::StringRef NewName = Case.NewName;
- auto Results = rename({T.point(),
- NewName,
- AST,
- testPath(TU.Filename),
- Case.Index,
- {/*CrossFile=*/false}});
+ auto Results = rename({T.point(), NewName, AST, testPath(TU.Filename)});
bool WantRename = true;
if (T.ranges().empty())
WantRename = false;
@@ -1176,10 +1092,8 @@
auto GetDirtyBuffer = [&](PathRef Path) -> llvm::Optional<std::string> {
return Code.code().str(); // Every file has the same content.
};
- RenameOptions Opts;
- Opts.AllowCrossFile = true;
- RenameInputs Inputs{Code.point(), "xPrime", AST, Main,
- Idx, Opts, GetDirtyBuffer};
+ RenameInputs Inputs{Code.point(), "xPrime", AST, Main,
+ Idx, RenameOptions(), GetDirtyBuffer};
auto Results = rename(Inputs);
EXPECT_TRUE(bool(Results)) << llvm::toString(Results.takeError());
return std::move(*Results);
@@ -1273,7 +1187,7 @@
runAddDocument(Server, FooCCPath, FooCC.code());
auto Results = runPrepareRename(Server, FooCCPath, FooCC.point(),
- /*NewName=*/llvm::None, {/*CrossFile=*/true});
+ /*NewName=*/llvm::None, {});
// Verify that for multi-file rename, we only return main-file occurrences.
ASSERT_TRUE(bool(Results)) << Results.takeError();
// We don't know the result is complete in prepareRename (passing a nullptr
@@ -1283,21 +1197,13 @@
testing::UnorderedElementsAreArray(Results->LocalChanges));
// Name validation.
- Results =
- runPrepareRename(Server, FooCCPath, FooCC.point(),
- /*NewName=*/std::string("int"), {/*CrossFile=*/true});
+ Results = runPrepareRename(Server, FooCCPath, FooCC.point(),
+ /*NewName=*/std::string("int"), {});
EXPECT_FALSE(Results);
EXPECT_THAT(llvm::toString(Results.takeError()),
testing::HasSubstr("keyword"));
EXPECT_THAT(Tracer.takeMetric("rename_name_invalid", "Keywords"),
ElementsAre(1));
-
- // Single-file rename on global symbols, we should report an error.
- Results = runPrepareRename(Server, FooCCPath, FooCC.point(),
- /*NewName=*/llvm::None, {/*CrossFile=*/false});
- EXPECT_FALSE(Results);
- EXPECT_THAT(llvm::toString(Results.takeError()),
- testing::HasSubstr("is used outside"));
}
TEST(CrossFileRenameTests, DirtyBuffer) {
@@ -1334,7 +1240,7 @@
AST,
MainFilePath,
Index.get(),
- {/*CrossFile=*/true},
+ {},
GetDirtyBuffer});
ASSERT_TRUE(bool(Results)) << Results.takeError();
EXPECT_THAT(
@@ -1355,7 +1261,7 @@
AST,
MainFilePath,
Index.get(),
- {/*CrossFile=*/true},
+ {},
GetDirtyBuffer});
ASSERT_TRUE(bool(Results)) << Results.takeError();
EXPECT_THAT(
@@ -1397,7 +1303,7 @@
AST,
MainFilePath,
&PIndex,
- {/*CrossFile=*/true},
+ {},
GetDirtyBuffer});
EXPECT_FALSE(Results);
EXPECT_THAT(llvm::toString(Results.takeError()),
@@ -1448,12 +1354,8 @@
Ref ReturnedRef;
} DIndex(XRefInBarCC);
llvm::StringRef NewName = "newName";
- auto Results = rename({MainCode.point(),
- NewName,
- AST,
- MainFilePath,
- &DIndex,
- {/*CrossFile=*/true}});
+ auto Results =
+ rename({MainCode.point(), NewName, AST, MainFilePath, &DIndex});
ASSERT_TRUE(bool(Results)) << Results.takeError();
EXPECT_THAT(
applyEdits(std::move(Results->GlobalChanges)),
@@ -1652,8 +1554,8 @@
llvm::StringRef NewName = "NewName";
for (const auto &RenamePos : FooH.points()) {
EXPECT_THAT(Tracer.takeMetric("rename_files"), SizeIs(0));
- auto FileEditsList = llvm::cantFail(runRename(
- Server, FooHPath, RenamePos, NewName, {/*CrossFile=*/true}));
+ auto FileEditsList =
+ llvm::cantFail(runRename(Server, FooHPath, RenamePos, NewName, {}));
EXPECT_THAT(Tracer.takeMetric("rename_files"), ElementsAre(2));
EXPECT_THAT(
applyEdits(std::move(FileEditsList.GlobalChanges)),
Index: clang-tools-extra/clangd/refactor/Rename.h
===================================================================
--- clang-tools-extra/clangd/refactor/Rename.h
+++ clang-tools-extra/clangd/refactor/Rename.h
@@ -27,9 +27,6 @@
llvm::function_ref<llvm::Optional<std::string>(PathRef AbsPath)>;
struct RenameOptions {
- /// If true, enable cross-file rename; otherwise, only allows to rename a
- /// symbol that's only used in the current file.
- bool AllowCrossFile = true;
/// The maximum number of affected files (0 means no limit), only meaningful
/// when AllowCrossFile = true.
/// If the actual number exceeds the limit, rename is forbidden.
Index: clang-tools-extra/clangd/refactor/Rename.cpp
===================================================================
--- clang-tools-extra/clangd/refactor/Rename.cpp
+++ clang-tools-extra/clangd/refactor/Rename.cpp
@@ -12,7 +12,6 @@
#include "ParsedAST.h"
#include "Selection.h"
#include "SourceCode.h"
-#include "index/SymbolCollector.h"
#include "support/Logger.h"
#include "support/Trace.h"
#include "clang/AST/ASTContext.h"
@@ -60,27 +59,6 @@
return false;
}
-// Query the index to find some other files where the Decl is referenced.
-llvm::Optional<std::string> getOtherRefFile(const Decl &D, StringRef MainFile,
- const SymbolIndex &Index) {
- RefsRequest Req;
- // We limit the number of results, this is a correctness/performance
- // tradeoff. We expect the number of symbol references in the current file
- // is smaller than the limit.
- Req.Limit = 100;
- Req.IDs.insert(getSymbolID(&D));
- llvm::Optional<std::string> OtherFile;
- Index.refs(Req, [&](const Ref &R) {
- if (OtherFile)
- return;
- if (auto RefFilePath = filePath(R.Location, /*HintFilePath=*/MainFile)) {
- if (!pathEqual(*RefFilePath, MainFile))
- OtherFile = *RefFilePath;
- }
- });
- return OtherFile;
-}
-
// Canonical declarations help simplify the process of renaming. Examples:
// - Template's canonical decl is the templated declaration (i.e.
// ClassTemplateDecl is canonicalized to its child CXXRecordDecl,
@@ -194,8 +172,6 @@
enum class ReasonToReject {
NoSymbolFound,
NoIndexProvided,
- NonIndexable,
- UsedOutsideFile, // for within-file rename only.
UnsupportedSymbol,
AmbiguousSymbol,
@@ -206,8 +182,7 @@
llvm::Optional<ReasonToReject> renameable(const NamedDecl &RenameDecl,
StringRef MainFilePath,
- const SymbolIndex *Index,
- bool CrossFile) {
+ const SymbolIndex *Index) {
trace::Span Tracer("Renameable");
// Filter out symbols that are unsupported in both rename modes.
if (llvm::isa<NamespaceDecl>(&RenameDecl))
@@ -223,51 +198,8 @@
if (isExcluded(RenameDecl))
return ReasonToReject::UnsupportedSymbol;
- // Check whether the symbol being rename is indexable.
- auto &ASTCtx = RenameDecl.getASTContext();
- bool MainFileIsHeader = isHeaderFile(MainFilePath, ASTCtx.getLangOpts());
- bool DeclaredInMainFile =
- isInsideMainFile(RenameDecl.getBeginLoc(), ASTCtx.getSourceManager());
- bool IsMainFileOnly = true;
- if (MainFileIsHeader)
- // main file is a header, the symbol can't be main file only.
- IsMainFileOnly = false;
- else if (!DeclaredInMainFile)
- IsMainFileOnly = false;
- // If the symbol is not indexable, we disallow rename.
- if (!SymbolCollector::shouldCollectSymbol(
- RenameDecl, RenameDecl.getASTContext(), SymbolCollector::Options(),
- IsMainFileOnly))
- return ReasonToReject::NonIndexable;
-
- if (!CrossFile) {
- if (!DeclaredInMainFile)
- // We are sure the symbol is used externally, bail out early.
- return ReasonToReject::UsedOutsideFile;
-
- // If the symbol is declared in the main file (which is not a header), we
- // rename it.
- if (!MainFileIsHeader)
- return None;
-
- if (!Index)
- return ReasonToReject::NoIndexProvided;
-
- auto OtherFile = getOtherRefFile(RenameDecl, MainFilePath, *Index);
- // If the symbol is indexable and has no refs from other files in the index,
- // we rename it.
- if (!OtherFile)
- return None;
- // If the symbol is indexable and has refs from other files in the index,
- // we disallow rename.
- return ReasonToReject::UsedOutsideFile;
- }
-
- assert(CrossFile);
-
// FIXME: Renaming virtual methods requires to rename all overridens in
// subclasses, our index doesn't have this information.
- // Note: Within-file rename does support this through the AST.
if (const auto *S = llvm::dyn_cast<CXXMethodDecl>(&RenameDecl)) {
if (S->isVirtual())
return ReasonToReject::UnsupportedSymbol;
@@ -282,10 +214,6 @@
return "there is no symbol at the given location";
case ReasonToReject::NoIndexProvided:
return "no index provided";
- case ReasonToReject::UsedOutsideFile:
- return "the symbol is used outside main file";
- case ReasonToReject::NonIndexable:
- return "symbol may be used in other files (not eligible for indexing)";
case ReasonToReject::UnsupportedSymbol:
return "symbol is not a supported kind (e.g. namespace, macro)";
case ReasonToReject::AmbiguousSymbol:
@@ -769,8 +697,7 @@
if (Invalid)
return makeError(*Invalid);
- auto Reject = renameable(RenameDecl, RInputs.MainFilePath, RInputs.Index,
- Opts.AllowCrossFile);
+ auto Reject = renameable(RenameDecl, RInputs.MainFilePath, RInputs.Index);
if (Reject)
return makeError(*Reject);
@@ -795,7 +722,7 @@
// return the main file edit if this is a within-file rename or the symbol
// being renamed is function local.
- if (!Opts.AllowCrossFile || RenameDecl.getParentFunctionOrMethod()) {
+ if (RenameDecl.getParentFunctionOrMethod()) {
Result.GlobalChanges = FileEdits(
{std::make_pair(RInputs.MainFilePath, std::move(MainFileEdits))});
return Result;
@@ -804,7 +731,7 @@
// If the index is nullptr, we don't know the completeness of the result, so
// we don't populate the field GlobalChanges.
if (!RInputs.Index) {
- assert(Result.GlobalChanges.empty() && Opts.AllowCrossFile);
+ assert(Result.GlobalChanges.empty());
return Result;
}
Index: clang-tools-extra/clangd/ClangdServer.cpp
===================================================================
--- clang-tools-extra/clangd/ClangdServer.cpp
+++ clang-tools-extra/clangd/ClangdServer.cpp
@@ -397,20 +397,16 @@
const RenameOptions &RenameOpts,
Callback<RenameResult> CB) {
auto Action = [Pos, File = File.str(), CB = std::move(CB),
- NewName = std::move(NewName), RenameOpts,
- this](llvm::Expected<InputsAndAST> InpAST) mutable {
+ NewName = std::move(NewName),
+ RenameOpts](llvm::Expected<InputsAndAST> InpAST) mutable {
if (!InpAST)
return CB(InpAST.takeError());
- // prepareRename is latency-sensitive:
- // - for single-file rename, performing rename isn't substantially more
- // expensive than doing an AST-based check (the index is used to see if
- // the rename is complete);
- // - for cross-file rename, we deliberately pass a nullptr index to save
- // the cost, thus the result may be incomplete as it only contains
- // main-file occurrences;
+ // prepareRename is latency-sensitive: we deliberately pass a nullptr index
+ // to save the cost, thus the result may be incomplete as it only contains
+ // main-file occurrences.
auto Results = clangd::rename(
{Pos, NewName.getValueOr("__clangd_rename_dummy"), InpAST->AST, File,
- RenameOpts.AllowCrossFile ? nullptr : Index, RenameOpts});
+ /*Index=*/nullptr, RenameOpts});
if (!Results) {
// LSP says to return null on failure, but that will result in a generic
// failure message. If we send an LSP error response, clients can surface
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
