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