[clang-tools-extra] a200501 - [clangd] Addusing tweak: find insertion point after definition
Adam Czachorowski via cfe-commits
cfe-commits at lists.llvm.org
Tue Nov 24 14:02:14 PST 2020
Author: Adam Czachorowski
Date: 2020-11-24T22:57:02+01:00
New Revision: a200501bca4dc7f3292d824f249fa21a479e9873
URL: https://github.com/llvm/llvm-project/commit/a200501bca4dc7f3292d824f249fa21a479e9873
DIFF: https://github.com/llvm/llvm-project/commit/a200501bca4dc7f3292d824f249fa21a479e9873.diff
LOG: [clangd] Addusing tweak: find insertion point after definition
When type/function is defined in the middle of the file, previuosly we
would sometimes insert a "using" line before that definition, leading to
a compilation error. With this fix, we pick a point after such
definition in translation unit.
This is not a perfect solution. For example, it still doesn't handle
"using namespace" directives. It is, however, a significant improvement.
Differential Revision: https://reviews.llvm.org/D92053
Added:
Modified:
clang-tools-extra/clangd/refactor/tweaks/AddUsing.cpp
clang-tools-extra/clangd/unittests/TweakTests.cpp
Removed:
################################################################################
diff --git a/clang-tools-extra/clangd/refactor/tweaks/AddUsing.cpp b/clang-tools-extra/clangd/refactor/tweaks/AddUsing.cpp
index b53df446c732..4b3fbc02c411 100644
--- a/clang-tools-extra/clangd/refactor/tweaks/AddUsing.cpp
+++ b/clang-tools-extra/clangd/refactor/tweaks/AddUsing.cpp
@@ -48,6 +48,10 @@ class AddUsing : public Tweak {
NestedNameSpecifierLoc QualifierToRemove;
// The name following QualifierToRemove.
llvm::StringRef Name;
+ // If valid, the insertion point for "using" statement must come after this.
+ // This is relevant when the type is defined in the main file, to make sure
+ // the type/function is already defined at the point where "using" is added.
+ SourceLocation MustInsertAfterLoc;
};
REGISTER_TWEAK(AddUsing)
@@ -120,7 +124,8 @@ struct InsertionPointData {
llvm::Expected<InsertionPointData>
findInsertionPoint(const Tweak::Selection &Inputs,
const NestedNameSpecifierLoc &QualifierToRemove,
- const llvm::StringRef Name) {
+ const llvm::StringRef Name,
+ const SourceLocation MustInsertAfterLoc) {
auto &SM = Inputs.AST->getSourceManager();
// Search for all using decls that affect this point in file. We need this for
@@ -132,6 +137,11 @@ findInsertionPoint(const Tweak::Selection &Inputs,
SM)
.TraverseAST(Inputs.AST->getASTContext());
+ auto IsValidPoint = [&](const SourceLocation Loc) {
+ return MustInsertAfterLoc.isInvalid() ||
+ SM.isBeforeInTranslationUnit(MustInsertAfterLoc, Loc);
+ };
+
bool AlwaysFullyQualify = true;
for (auto &U : Usings) {
// Only "upgrade" to fully qualified is all relevant using decls are fully
@@ -149,12 +159,13 @@ findInsertionPoint(const Tweak::Selection &Inputs,
U->getName() == Name) {
return InsertionPointData();
}
+
// Insertion point will be before last UsingDecl that affects cursor
// position. For most cases this should stick with the local convention of
// add using inside or outside namespace.
LastUsingLoc = U->getUsingLoc();
}
- if (LastUsingLoc.isValid()) {
+ if (LastUsingLoc.isValid() && IsValidPoint(LastUsingLoc)) {
InsertionPointData Out;
Out.Loc = LastUsingLoc;
Out.AlwaysFullyQualify = AlwaysFullyQualify;
@@ -175,7 +186,7 @@ findInsertionPoint(const Tweak::Selection &Inputs,
if (Tok == Toks.end() || Tok->endLocation().isInvalid()) {
return error("Namespace with no {");
}
- if (!Tok->endLocation().isMacroID()) {
+ if (!Tok->endLocation().isMacroID() && IsValidPoint(Tok->endLocation())) {
InsertionPointData Out;
Out.Loc = Tok->endLocation();
Out.Suffix = "\n";
@@ -183,15 +194,17 @@ findInsertionPoint(const Tweak::Selection &Inputs,
}
}
// No using, no namespace, no idea where to insert. Try above the first
- // top level decl.
+ // top level decl after MustInsertAfterLoc.
auto TLDs = Inputs.AST->getLocalTopLevelDecls();
- if (TLDs.empty()) {
- return error("Cannot find place to insert \"using\"");
+ for (const auto &TLD : TLDs) {
+ if (!IsValidPoint(TLD->getBeginLoc()))
+ continue;
+ InsertionPointData Out;
+ Out.Loc = SM.getExpansionLoc(TLD->getBeginLoc());
+ Out.Suffix = "\n\n";
+ return Out;
}
- InsertionPointData Out;
- Out.Loc = SM.getExpansionLoc(TLDs[0]->getBeginLoc());
- Out.Suffix = "\n\n";
- return Out;
+ return error("Cannot find place to insert \"using\"");
}
bool isNamespaceForbidden(const Tweak::Selection &Inputs,
@@ -254,6 +267,7 @@ bool AddUsing::prepare(const Selection &Inputs) {
if (auto *II = D->getDecl()->getIdentifier()) {
QualifierToRemove = D->getQualifierLoc();
Name = II->getName();
+ MustInsertAfterLoc = D->getDecl()->getBeginLoc();
}
} else if (auto *T = Node->ASTNode.get<TypeLoc>()) {
if (auto E = T->getAs<ElaboratedTypeLoc>()) {
@@ -271,6 +285,15 @@ bool AddUsing::prepare(const Selection &Inputs) {
QualifierToRemove, Inputs.AST->getASTContext().getPrintingPolicy());
if (!Name.consume_front(QualifierToRemoveStr))
return false; // What's spelled doesn't match the qualifier.
+
+ if (const auto *ET = E.getTypePtr()) {
+ if (const auto *TDT =
+ dyn_cast<TypedefType>(ET->getNamedType().getTypePtr())) {
+ MustInsertAfterLoc = TDT->getDecl()->getBeginLoc();
+ } else if (auto *TD = ET->getAsTagDecl()) {
+ MustInsertAfterLoc = TD->getBeginLoc();
+ }
+ }
}
}
@@ -312,7 +335,8 @@ Expected<Tweak::Effect> AddUsing::apply(const Selection &Inputs) {
return std::move(Err);
}
- auto InsertionPoint = findInsertionPoint(Inputs, QualifierToRemove, Name);
+ auto InsertionPoint =
+ findInsertionPoint(Inputs, QualifierToRemove, Name, MustInsertAfterLoc);
if (!InsertionPoint) {
return InsertionPoint.takeError();
}
diff --git a/clang-tools-extra/clangd/unittests/TweakTests.cpp b/clang-tools-extra/clangd/unittests/TweakTests.cpp
index edfaee779e38..fd815d2c4c27 100644
--- a/clang-tools-extra/clangd/unittests/TweakTests.cpp
+++ b/clang-tools-extra/clangd/unittests/TweakTests.cpp
@@ -2825,7 +2825,66 @@ using namespace one;
namespace {using two::cc;
cc C;
-})cpp"}};
+})cpp"},
+ // Type defined in main file, make sure using is after that.
+ {R"cpp(
+namespace xx {
+ struct yy {};
+}
+
+x^x::yy X;
+)cpp",
+ R"cpp(
+namespace xx {
+ struct yy {};
+}
+
+using xx::yy;
+
+yy X;
+)cpp"},
+ // Type defined in main file via "using", insert after that.
+ {R"cpp(
+#include "test.hpp"
+
+namespace xx {
+ using yy = one::two::cc;
+}
+
+x^x::yy X;
+)cpp",
+ R"cpp(
+#include "test.hpp"
+
+namespace xx {
+ using yy = one::two::cc;
+}
+
+using xx::yy;
+
+yy X;
+)cpp"},
+ // Using must come after function definition.
+ {R"cpp(
+namespace xx {
+ void yy();
+}
+
+void fun() {
+ x^x::yy();
+}
+)cpp",
+ R"cpp(
+namespace xx {
+ void yy();
+}
+
+using xx::yy;
+
+void fun() {
+ yy();
+}
+)cpp"}};
llvm::StringMap<std::string> EditedFiles;
for (const auto &Case : Cases) {
for (const auto &SubCase : expandCases(Case.TestSource)) {
More information about the cfe-commits
mailing list