[PATCH] D92053: [clangd] Addusing tweak: find insertion point after definition

[Adam Czachorowski via Phabricator via cfe-commits]

adamcz updated this revision to Diff 307429.
adamcz added a comment.

typo in description


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D92053/new/

https://reviews.llvm.org/D92053

Files:
  clang-tools-extra/clangd/refactor/tweaks/AddUsing.cpp
  clang-tools-extra/clangd/unittests/TweakTests.cpp


Index: clang-tools-extra/clangd/unittests/TweakTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/TweakTests.cpp
+++ clang-tools-extra/clangd/unittests/TweakTests.cpp
@@ -2825,7 +2825,66 @@
 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)) {
Index: clang-tools-extra/clangd/refactor/tweaks/AddUsing.cpp
===================================================================
--- clang-tools-extra/clangd/refactor/tweaks/AddUsing.cpp
+++ clang-tools-extra/clangd/refactor/tweaks/AddUsing.cpp
@@ -48,6 +48,10 @@
   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 @@
 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
@@ -149,6 +154,10 @@
         U->getName() == Name) {
       return InsertionPointData();
     }
+
+    if (MustInsertAfterLoc.isValid() &&
+        SM.isBeforeInTranslationUnit(U->getUsingLoc(), MustInsertAfterLoc))
+      continue;
     // 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.
@@ -175,7 +184,10 @@
     if (Tok == Toks.end() || Tok->endLocation().isInvalid()) {
       return error("Namespace with no {");
     }
-    if (!Tok->endLocation().isMacroID()) {
+    if (!Tok->endLocation().isMacroID() &&
+        (MustInsertAfterLoc.isInvalid() ||
+         SM.isBeforeInTranslationUnit(MustInsertAfterLoc,
+                                      Tok->endLocation()))) {
       InsertionPointData Out;
       Out.Loc = Tok->endLocation();
       Out.Suffix = "\n";
@@ -183,15 +195,18 @@
     }
   }
   // 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 (MustInsertAfterLoc.isValid() &&
+        SM.isBeforeInTranslationUnit(TLD->getBeginLoc(), MustInsertAfterLoc))
+      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 +269,7 @@
     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 +287,15 @@
           QualifierToRemove, Inputs.AST->getASTContext().getPrintingPolicy());
       if (!Name.consume_front(QualifierToRemoveStr))
         return false; // What's spelled doesn't match the qualifier.
+
+      if (auto *ET = E.getTypePtr()) {
+        if (auto *TDT =
+                dyn_cast<TypedefType>(ET->getNamedType().getTypePtr())) {
+          MustInsertAfterLoc = TDT->getDecl()->getBeginLoc();
+        } else if (auto *TD = ET->getAsTagDecl()) {
+          MustInsertAfterLoc = TD->getBeginLoc();
+        }
+      }
     }
   }
 
@@ -312,7 +337,8 @@
     return std::move(Err);
   }
 
-  auto InsertionPoint = findInsertionPoint(Inputs, QualifierToRemove, Name);
+  auto InsertionPoint =
+      findInsertionPoint(Inputs, QualifierToRemove, Name, MustInsertAfterLoc);
   if (!InsertionPoint) {
     return InsertionPoint.takeError();
   }

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits