[PATCH] D76432: [clangd] Add a tweak for adding "using" statement.

[Sam McCall via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG24bb2d1e7768: [clangd] Add a tweak for adding 
using statement. (authored by adamcz, committed by sammccall).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76432

Files:
  clang-tools-extra/clangd/refactor/tweaks/AddUsing.cpp
  clang-tools-extra/clangd/refactor/tweaks/CMakeLists.txt
  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
@@ -2390,6 +2390,250 @@
 EXPECT_EQ(apply(Case.first), Case.second);
   }
 }
+
+TWEAK_TEST(AddUsing);
+TEST_F(AddUsingTest, Prepare) {
+  const std::string Header = R"cpp(
+#define NS(name) one::two::name
+namespace one {
+void oo() {}
+namespace two {
+enum ee {};
+void ff() {}
+class cc {
+public:
+  struct st {};
+  static void mm() {}
+};
+}
+})cpp";
+
+  EXPECT_AVAILABLE(Header + "void fun() { o^n^e^:^:^t^w^o^:^:^f^f(); }");
+  EXPECT_AVAILABLE(Header + "void fun() { o^n^e^::^o^o(); }");
+  EXPECT_AVAILABLE(Header + "void fun() { o^n^e^:^:^t^w^o^:^:^e^e E; }");
+  EXPECT_AVAILABLE(Header + "void fun() { o^n^e^:^:^t^w^o:^:^c^c C; }");
+  EXPECT_UNAVAILABLE(Header +
+ "void fun() { o^n^e^:^:^t^w^o^:^:^c^c^:^:^m^m(); }");
+  EXPECT_UNAVAILABLE(Header +
+ "void fun() { o^n^e^:^:^t^w^o^:^:^c^c^:^:^s^t inst; }");
+  EXPECT_UNAVAILABLE(Header +
+ "void fun() { o^n^e^:^:^t^w^o^:^:^c^c^:^:^s^t inst; }");
+  EXPECT_UNAVAILABLE(Header + "void fun() { N^S(c^c) inst; }");
+}
+
+TEST_F(AddUsingTest, Apply) {
+  FileName = "test.cpp";
+  struct {
+llvm::StringRef TestSource;
+llvm::StringRef ExpectedSource;
+  } Cases[]{{
+// Function, no other using, namespace.
+R"cpp(
+#include "test.hpp"
+namespace {
+void fun() {
+  ^o^n^e^:^:^t^w^o^:^:^f^f();
+}
+})cpp",
+R"cpp(
+#include "test.hpp"
+namespace {using one::two::ff;
+
+void fun() {
+  ff();
+}
+})cpp",
+},
+// Type, no other using, namespace.
+{
+R"cpp(
+#include "test.hpp"
+namespace {
+void fun() {
+  ::on^e::t^wo::c^c inst;
+}
+})cpp",
+R"cpp(
+#include "test.hpp"
+namespace {using ::one::two::cc;
+
+void fun() {
+  cc inst;
+}
+})cpp",
+},
+// Type, no other using, no namespace.
+{
+R"cpp(
+#include "test.hpp"
+
+void fun() {
+  on^e::t^wo::e^e inst;
+})cpp",
+R"cpp(
+#include "test.hpp"
+
+using one::two::ee;
+
+void fun() {
+  ee inst;
+})cpp"},
+// Function, other usings.
+{
+R"cpp(
+#include "test.hpp"
+
+using one::two::cc;
+using one::two::ee;
+
+namespace {
+void fun() {
+  one::two::f^f();
+}
+})cpp",
+R"cpp(
+#include "test.hpp"
+
+using one::two::cc;
+using one::two::ff;using one::two::ee;
+
+namespace {
+void fun() {
+  ff();
+}
+})cpp",
+},
+// Function, other usings inside namespace.
+{
+R"cpp(
+#include "test.hpp"
+
+using one::two::cc;
+
+namespace {
+
+using one::two::ff;
+
+void fun() {
+  o^ne::o^o();
+}
+})cpp",
+R"cpp(
+#include "test.hpp"
+
+using one::two::cc;
+
+namespace {
+
+using one::oo;using one::two::ff;
+
+void fun() {
+  oo();
+}
+})cpp"},
+// Using comes after cursor.
+{
+R"cpp(
+#include "test.hpp"
+
+namespace {
+
+void fun() {
+  one::t^wo::ff();
+}
+
+using one::two::cc;
+
+})cpp",
+R"cpp(
+#include "test.hpp"
+
+namespace {using one::two::ff;
+
+
+void fun() {
+  ff();
+}
+
+using one::two::cc;
+
+})cpp"},
+// Pointer type.
+{R"cpp(
+#include "test.hpp"
+
+void fun() {
+  one::two::c^c *p;
+})cpp",
+ R"cpp(
+#include "test.hpp"
+
+using one::two::cc;
+
+void fun() {
+  cc *p;
+})cpp"},
+// Namespace declared via macro.
+{R"cpp(
+#include "test.hpp"
+#define NS_BEGIN(name) namespace name {
+
+NS_BEGIN(foo)
+
+void fun() {
+  one::two::f^f();
+}
+})cpp",
+ R"cpp(
+#include "test.hpp"
+#define NS_BEGIN(name) namespace name {
+
+using one::two::ff;
+
+NS_BEGIN(foo)
+
+void fun() {
+  ff();
+}
+})cpp"},
+// Inside macro argument.
+{R"cpp(
+#include "test.hpp"
+#define CALL(name) name()
+
+void fun() {
+  CALL(one::t^wo::ff);
+})cpp",
+ R"cpp(
+#include "test.hpp"
+#define CALL(name) name()
+
+using one::two::ff;
+
+void fun() {
+  CALL(ff);
+})cpp"}};
+  llvm::StringMap EditedFiles;
+  for (const auto  : Cases) {
+for (const auto  : expandCases(Case.TestSource)) {
+  ExtraFiles["test.hpp"] = R"cpp(
+namespace one {
+void oo() {}
+namespace two {
+enum 

[PATCH] D76432: [clangd] Add a tweak for adding "using" statement.

[Adam Czachorowski via Phabricator via cfe-commits]

adamcz added a comment.

In D76432#1952798 , @sammccall wrote:

> Still LG, should I land this?


Yes please :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76432



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

[PATCH] D76432: [clangd] Add a tweak for adding "using" statement.

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Still LG, should I land this?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76432



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

[PATCH] D76432: [clangd] Add a tweak for adding "using" statement.

[Adam Czachorowski via Phabricator via cfe-commits]

adamcz added inline comments.



Comment at: clang-tools-extra/clangd/refactor/tweaks/AddUsing.cpp:89
+
+  // If we're looking at a type or NestedNameSpecifier, walk up the tree until
+  // we find the "main" node we care about, which would be ElaboratedTypeLoc or

sammccall wrote:
> adamcz wrote:
> > sammccall wrote:
> > > I like the idea here, but I'm not sure it quite works. e.g. any typeloc 
> > > has a directly enclosing typeloc, the inner one can't be targeted. So 
> > > what about `x::S*`? (pointertypeloc > elaboratedtypeloc > recordtypeloc)?
> > > 
> > > - looping up from NNS until you get to something else makes sense
> > > - unwrapping typeloc -> elaboratedtypeloc makes sense, but I don't think 
> > > you ever want to unwrap multiple levels?
> > > - i don't think these cases overlap at all, you want one or the other
> > > 
> > > Am I missing something?
> > If you have a class foo::bar::cc and a struct st inside it, and then do:
> > foo::bar::c^c::st *p;
> > you end up with:
> > PointerTypeLoc -> ElaboratedTypeLoc ->NestedNameSpecifierLoc -> 
> > RecordTypeLoc
> > in which case you need to go up from type to NNSL and then up again, from 
> > NNSL to something that's not NNSL.
> > 
> > You have a good point with the PointerTypeLoc, that's a bug. We should stop 
> > going up the tree as soon as we find ElaboratedTypeLoc. I added a test for 
> > that.
> > foo::bar::c^c::st *p;
> 
> But this isn't a case we support, right? We only support extraction when the 
> NNS consists entirely of namespaces, and such NNSes don't have any children 
> apart from the qualifier NNS.
Not right now, but I left a comment that we should improve that. The way it is 
now, code is correct, always, and improving it is just a matter or removing the 
"no record types" check and replacing with a bit more code. Seems better to me.



Comment at: clang-tools-extra/clangd/refactor/tweaks/AddUsing.cpp:206
+  tooling::Replacements R;
+  if (auto Err = R.add(tooling::Replacement(
+  SM, CharSourceRange::getTokenRange(NNSL.getSourceRange()), "",

kadircet wrote:
> sammccall wrote:
> > adamcz wrote:
> > > sammccall wrote:
> > > > (Sorry if some of this is obvious: TL;DR: we should use TokenBuffer to 
> > > > handle macros in this case)
> > > > 
> > > > Whenever we try to use SourceLocations to reason about written source 
> > > > code, we have to think about macros. Some libraries try to encapsulate 
> > > > this, but the road to confusion is paved with good intentions, and it's 
> > > > often best to decide on the policy and be explicit about it.
> > > > 
> > > > For example, when provided a macro location, the tooling::Replacement 
> > > > constructor uses its spelling location. 
> > > > Given: 
> > > > ```
> > > > // imagine we're trying to abstract away namespaces for C or something
> > > > #define NS(X) foo::X
> > > > NS(Bar) boo;
> > > > ```
> > > > Running this action on `N` would result in changing the definition of 
> > > > the `NS` macro to delete the "foo::", as that's where the qualifier was 
> > > > spelled!
> > > > 
> > > > It would be reasonable (but actually not trivial!) to try to detect any 
> > > > macro usage and bail out. The general case of "is there some sequence 
> > > > of source-code tokens that correspond to this range of preprocessed 
> > > > tokens and nothing else" is pretty hard.
> > > > 
> > > > However TokenBuffer does have APIs for this exact question, as it was 
> > > > designed for writing refactorings that replace nodes. I think you want:
> > > >  - `expandedTokens(NNSL.getSourceRange())`
> > > >  - `spelledForExpanded(ExpandedTokens)`
> > > >  - `Token::range(SM, Spelled.front(), Spelled.back())`
> > > >  - `Replacement("fname", Range.Offset, Range.Length, "")`
> > > > 
> > > > (ugh, that's really awkward. Maybe we should have a helper in 
> > > > TokenBuffer that combines 1-3)
> > > > 
> > > You have a good point that this doesn't work well with macros, but I'm 
> > > not entirely sure how you think this should work.
> > > 
> > > I'd argue that code actions should refer to the thing under the cursor, 
> > > not it's expansion. That would be confusing to the user, as they would 
> > > not be able to understand what the action offered is, nor how it would 
> > > affect other places. So in your example of 
> > > #define NS(X) foo::X
> > > I'd argue that we should not offer the action. 
> > > We should, however, support something like:
> > > EXPECT_TRUE(fo^o::bar());
> > > In this case, the qualifier is spelled under the cursor and the fact that 
> > > EXPECT_TRUE is a macro and not a function should not matter to the user.
> > > 
> > > I updated the code to support that and added tests.
> > > We can use isMacroID() and isMacroArgExpansion() to filter out macros, 
> > > except for macro arguments. Note that we can't use spelledForExpanded(), 
> > > since the qualifier might not be the entire expansion of the macro, thus 
> > > 

[PATCH] D76432: [clangd] Add a tweak for adding "using" statement.

[Adam Czachorowski via Phabricator via cfe-commits]

adamcz updated this revision to Diff 253583.
adamcz marked 10 inline comments as done.
adamcz added a comment.

review round 2


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76432

Files:
  clang-tools-extra/clangd/refactor/tweaks/AddUsing.cpp
  clang-tools-extra/clangd/refactor/tweaks/CMakeLists.txt
  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
@@ -2390,6 +2390,250 @@
 EXPECT_EQ(apply(Case.first), Case.second);
   }
 }
+
+TWEAK_TEST(AddUsing);
+TEST_F(AddUsingTest, Prepare) {
+  const std::string Header = R"cpp(
+#define NS(name) one::two::name
+namespace one {
+void oo() {}
+namespace two {
+enum ee {};
+void ff() {}
+class cc {
+public:
+  struct st {};
+  static void mm() {}
+};
+}
+})cpp";
+
+  EXPECT_AVAILABLE(Header + "void fun() { o^n^e^:^:^t^w^o^:^:^f^f(); }");
+  EXPECT_AVAILABLE(Header + "void fun() { o^n^e^::^o^o(); }");
+  EXPECT_AVAILABLE(Header + "void fun() { o^n^e^:^:^t^w^o^:^:^e^e E; }");
+  EXPECT_AVAILABLE(Header + "void fun() { o^n^e^:^:^t^w^o:^:^c^c C; }");
+  EXPECT_UNAVAILABLE(Header +
+ "void fun() { o^n^e^:^:^t^w^o^:^:^c^c^:^:^m^m(); }");
+  EXPECT_UNAVAILABLE(Header +
+ "void fun() { o^n^e^:^:^t^w^o^:^:^c^c^:^:^s^t inst; }");
+  EXPECT_UNAVAILABLE(Header +
+ "void fun() { o^n^e^:^:^t^w^o^:^:^c^c^:^:^s^t inst; }");
+  EXPECT_UNAVAILABLE(Header + "void fun() { N^S(c^c) inst; }");
+}
+
+TEST_F(AddUsingTest, Apply) {
+  FileName = "test.cpp";
+  struct {
+llvm::StringRef TestSource;
+llvm::StringRef ExpectedSource;
+  } Cases[]{{
+// Function, no other using, namespace.
+R"cpp(
+#include "test.hpp"
+namespace {
+void fun() {
+  ^o^n^e^:^:^t^w^o^:^:^f^f();
+}
+})cpp",
+R"cpp(
+#include "test.hpp"
+namespace {using one::two::ff;
+
+void fun() {
+  ff();
+}
+})cpp",
+},
+// Type, no other using, namespace.
+{
+R"cpp(
+#include "test.hpp"
+namespace {
+void fun() {
+  ::on^e::t^wo::c^c inst;
+}
+})cpp",
+R"cpp(
+#include "test.hpp"
+namespace {using ::one::two::cc;
+
+void fun() {
+  cc inst;
+}
+})cpp",
+},
+// Type, no other using, no namespace.
+{
+R"cpp(
+#include "test.hpp"
+
+void fun() {
+  on^e::t^wo::e^e inst;
+})cpp",
+R"cpp(
+#include "test.hpp"
+
+using one::two::ee;
+
+void fun() {
+  ee inst;
+})cpp"},
+// Function, other usings.
+{
+R"cpp(
+#include "test.hpp"
+
+using one::two::cc;
+using one::two::ee;
+
+namespace {
+void fun() {
+  one::two::f^f();
+}
+})cpp",
+R"cpp(
+#include "test.hpp"
+
+using one::two::cc;
+using one::two::ff;using one::two::ee;
+
+namespace {
+void fun() {
+  ff();
+}
+})cpp",
+},
+// Function, other usings inside namespace.
+{
+R"cpp(
+#include "test.hpp"
+
+using one::two::cc;
+
+namespace {
+
+using one::two::ff;
+
+void fun() {
+  o^ne::o^o();
+}
+})cpp",
+R"cpp(
+#include "test.hpp"
+
+using one::two::cc;
+
+namespace {
+
+using one::oo;using one::two::ff;
+
+void fun() {
+  oo();
+}
+})cpp"},
+// Using comes after cursor.
+{
+R"cpp(
+#include "test.hpp"
+
+namespace {
+
+void fun() {
+  one::t^wo::ff();
+}
+
+using one::two::cc;
+
+})cpp",
+R"cpp(
+#include "test.hpp"
+
+namespace {using one::two::ff;
+
+
+void fun() {
+  ff();
+}
+
+using one::two::cc;
+
+})cpp"},
+// Pointer type.
+{R"cpp(
+#include "test.hpp"
+
+void fun() {
+  one::two::c^c *p;
+})cpp",
+ R"cpp(
+#include "test.hpp"
+
+using one::two::cc;
+
+void fun() {
+  cc *p;
+})cpp"},
+// Namespace declared via macro.
+{R"cpp(
+#include "test.hpp"
+#define NS_BEGIN(name) namespace name {
+
+NS_BEGIN(foo)
+
+void fun() {
+  one::two::f^f();
+}
+})cpp",
+ R"cpp(
+#include "test.hpp"
+#define NS_BEGIN(name) namespace name {
+
+using one::two::ff;
+
+NS_BEGIN(foo)
+
+void fun() {
+  ff();
+}
+})cpp"},
+// Inside macro argument.
+{R"cpp(
+#include "test.hpp"
+#define CALL(name) name()
+
+void fun() {
+  CALL(one::t^wo::ff);
+})cpp",
+ R"cpp(
+#include "test.hpp"
+#define CALL(name) name()
+
+using one::two::ff;
+
+void fun() {
+  CALL(ff);
+})cpp"}};
+  llvm::StringMap EditedFiles;
+  for (const auto  : Cases) {
+for (const auto  : expandCases(Case.TestSource)) {
+  ExtraFiles["test.hpp"] = R"cpp(
+namespace one {
+void oo() {}
+namespace two {
+enum ee {};
+void ff() {}
+class cc {
+public:
+  struct st { struct nested {}; 

[PATCH] D76432: [clangd] Add a tweak for adding "using" statement.

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.



Comment at: clang-tools-extra/clangd/refactor/tweaks/AddUsing.cpp:206
+  tooling::Replacements R;
+  if (auto Err = R.add(tooling::Replacement(
+  SM, CharSourceRange::getTokenRange(NNSL.getSourceRange()), "",

sammccall wrote:
> adamcz wrote:
> > sammccall wrote:
> > > (Sorry if some of this is obvious: TL;DR: we should use TokenBuffer to 
> > > handle macros in this case)
> > > 
> > > Whenever we try to use SourceLocations to reason about written source 
> > > code, we have to think about macros. Some libraries try to encapsulate 
> > > this, but the road to confusion is paved with good intentions, and it's 
> > > often best to decide on the policy and be explicit about it.
> > > 
> > > For example, when provided a macro location, the tooling::Replacement 
> > > constructor uses its spelling location. 
> > > Given: 
> > > ```
> > > // imagine we're trying to abstract away namespaces for C or something
> > > #define NS(X) foo::X
> > > NS(Bar) boo;
> > > ```
> > > Running this action on `N` would result in changing the definition of the 
> > > `NS` macro to delete the "foo::", as that's where the qualifier was 
> > > spelled!
> > > 
> > > It would be reasonable (but actually not trivial!) to try to detect any 
> > > macro usage and bail out. The general case of "is there some sequence of 
> > > source-code tokens that correspond to this range of preprocessed tokens 
> > > and nothing else" is pretty hard.
> > > 
> > > However TokenBuffer does have APIs for this exact question, as it was 
> > > designed for writing refactorings that replace nodes. I think you want:
> > >  - `expandedTokens(NNSL.getSourceRange())`
> > >  - `spelledForExpanded(ExpandedTokens)`
> > >  - `Token::range(SM, Spelled.front(), Spelled.back())`
> > >  - `Replacement("fname", Range.Offset, Range.Length, "")`
> > > 
> > > (ugh, that's really awkward. Maybe we should have a helper in TokenBuffer 
> > > that combines 1-3)
> > > 
> > You have a good point that this doesn't work well with macros, but I'm not 
> > entirely sure how you think this should work.
> > 
> > I'd argue that code actions should refer to the thing under the cursor, not 
> > it's expansion. That would be confusing to the user, as they would not be 
> > able to understand what the action offered is, nor how it would affect 
> > other places. So in your example of 
> > #define NS(X) foo::X
> > I'd argue that we should not offer the action. 
> > We should, however, support something like:
> > EXPECT_TRUE(fo^o::bar());
> > In this case, the qualifier is spelled under the cursor and the fact that 
> > EXPECT_TRUE is a macro and not a function should not matter to the user.
> > 
> > I updated the code to support that and added tests.
> > We can use isMacroID() and isMacroArgExpansion() to filter out macros, 
> > except for macro arguments. Note that we can't use spelledForExpanded(), 
> > since the qualifier might not be the entire expansion of the macro, thus 
> > spelledForExpanded will return None.
> > 
> > Let me know if any of what I did here now doesn't make sense.
> > 
> > in your example of #define NS(X) foo::X I'd argue that we should not offer 
> > the action.
> 
> Indeed, sorry I didn't spell that out - I was highlighting it because the 
> original code silently did something (bad) in this case.
> 
> > can't use spelledForExpanded(), since the qualifier might not be the entire 
> > expansion of the macro
> 
> Ah, D75446 hasn't landed yet. @kadircet, want to land this soon? :-)
> 
> 
done, let me know if it doesn't work :D



Comment at: clang-tools-extra/clangd/refactor/tweaks/AddUsing.cpp:249
+  const auto *SpelledBeginTok =
+  TB.spelledTokenAt(SM.getSpellingLoc(ExpandedTokens.front().location()));
+  const auto *SpelledEndTok =

why not use spelledForExpanded in here as well?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76432



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

[PATCH] D76432: [clangd] Add a tweak for adding "using" statement.

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.



Comment at: clang-tools-extra/clangd/refactor/tweaks/AddUsing.cpp:245
+  auto  = Inputs.AST->getSourceManager();
+  auto TB = Inputs.AST->getTokens();
+  // Determine the length of the qualifier under the cursor, then remove it.

sammccall wrote:
> yikes, this copies the tokenbuffer, I didn't think that was even possible!
> auto&
Copy constructor deleted in 94938d7d41cd11c4539ff93b801fe53cb4fddba2


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76432



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

[PATCH] D76432: [clangd] Add a tweak for adding "using" statement.

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.

LG, thanks! Let me know if/when I should land this for you.




Comment at: clang-tools-extra/clangd/refactor/tweaks/AddUsing.cpp:105
+// argument.
+bool isInMacroNotArg(const SourceManager , const SourceLocation Loc) {
+  return Loc.isMacroID() && !SM.isMacroArgExpansion(Loc);

This is just SourceManager::isMacroBodyExpansion(), I think.



Comment at: clang-tools-extra/clangd/refactor/tweaks/AddUsing.cpp:163
+  if (isInMacroNotArg(SM, QualifierToRemove.getBeginLoc()) ||
+  isInMacroNotArg(SM, QualifierToRemove.getEndLoc())) {
+return false;

I think the endloc check should rather be SM.isWrittenInSameFile - it's no good 
if e.g. they're both macro arg expansions, but different ones.



Comment at: clang-tools-extra/clangd/refactor/tweaks/AddUsing.cpp:171
+// The insertion point might be a little awkward if the decl we're anchoring to
+// has a comment in an unfortunate place (e.g. directly above using, or
+// immediately following "namespace {". We should add some helpers for dealing

nit: you've mentioned the two cases I don't think we should bother fixing, but 
not the crucial one: we're anchoring to a regular decl, and we're going to 
insert in between the decl and its doc comment.



Comment at: clang-tools-extra/clangd/refactor/tweaks/AddUsing.cpp:245
+  auto  = Inputs.AST->getSourceManager();
+  auto TB = Inputs.AST->getTokens();
+  // Determine the length of the qualifier under the cursor, then remove it.

yikes, this copies the tokenbuffer, I didn't think that was even possible!
auto&



Comment at: clang-tools-extra/clangd/refactor/tweaks/AddUsing.cpp:44
+  // SourceLocation if the "using" statement already exists.
+  llvm::Expected
+  findInsertionPoint(const Selection );

adamcz wrote:
> sammccall wrote:
> > this doesn't use any state - make it static or a free function?
> It uses Name and NNSL . Would you prefer this as free function, with both 
> passed as arguments. My initial thought was that, since prepare() and apply() 
> already share these via class members this was fine, but I'm certainly not 
> against making this a free function.
Oops, right. I think since this is the complicated part and it has a fairly 
simple interface, making its inputs/outputs explicit is nice hygiene.

(prepare and apply do communicate through class state, which feels a bit hacky 
to me. We didn't come up with anything nicer but still flexible+simple when 
designing this. It is conventional and documented at least)



Comment at: clang-tools-extra/clangd/refactor/tweaks/AddUsing.cpp:71
+  return true;
+}
+if (D->getDeclContext()->Encloses(SelectionDeclContext)) {

adamcz wrote:
> sammccall wrote:
> > nit: we conventionally leave out the {} on one-line if bodies etc.
> Uhh...is that a hard rule? I personally hate that, it's just asking for Apple 
> SSL-style bugs
> https://www.imperialviolet.org/2014/02/22/applebug.html
There's no hard rule, but we do use that style consistently and consistency has 
value too.

The clang-tidy check `readability-misleading-indentation` catches that bug. Can 
I suggest:
 - (for us) adding it to `.clang-tidy` in `llvm-project`? This will affect 
clangd and the phabricator linter
 - (for the world) we could start a list of on-by-default clang-tidy checks for 
clangd users with no `.clang-tidy` config, and include this check



Comment at: clang-tools-extra/clangd/refactor/tweaks/AddUsing.cpp:89
+
+  // If we're looking at a type or NestedNameSpecifier, walk up the tree until
+  // we find the "main" node we care about, which would be ElaboratedTypeLoc or

adamcz wrote:
> sammccall wrote:
> > I like the idea here, but I'm not sure it quite works. e.g. any typeloc has 
> > a directly enclosing typeloc, the inner one can't be targeted. So what 
> > about `x::S*`? (pointertypeloc > elaboratedtypeloc > recordtypeloc)?
> > 
> > - looping up from NNS until you get to something else makes sense
> > - unwrapping typeloc -> elaboratedtypeloc makes sense, but I don't think 
> > you ever want to unwrap multiple levels?
> > - i don't think these cases overlap at all, you want one or the other
> > 
> > Am I missing something?
> If you have a class foo::bar::cc and a struct st inside it, and then do:
> foo::bar::c^c::st *p;
> you end up with:
> PointerTypeLoc -> ElaboratedTypeLoc ->NestedNameSpecifierLoc -> RecordTypeLoc
> in which case you need to go up from type to NNSL and then up again, from 
> NNSL to something that's not NNSL.
> 
> You have a good point with the PointerTypeLoc, that's a bug. We should stop 
> going up the tree as soon as we find ElaboratedTypeLoc. I added a test for 
> that.
> foo::bar::c^c::st *p;

But this isn't a case we 

[PATCH] D76432: [clangd] Add a tweak for adding "using" statement.

[Adam Czachorowski via Phabricator via cfe-commits]

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

clang-format


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76432

Files:
  clang-tools-extra/clangd/refactor/tweaks/AddUsing.cpp
  clang-tools-extra/clangd/refactor/tweaks/CMakeLists.txt
  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
@@ -2390,6 +2390,250 @@
 EXPECT_EQ(apply(Case.first), Case.second);
   }
 }
+
+TWEAK_TEST(AddUsing);
+TEST_F(AddUsingTest, Prepare) {
+  const std::string Header = R"cpp(
+#define NS(name) one::two::name
+namespace one {
+void oo() {}
+namespace two {
+enum ee {};
+void ff() {}
+class cc {
+public:
+  struct st {};
+  static void mm() {}
+};
+}
+})cpp";
+
+  EXPECT_AVAILABLE(Header + "void fun() { o^n^e^:^:^t^w^o^:^:^f^f(); }");
+  EXPECT_AVAILABLE(Header + "void fun() { o^n^e^::^o^o(); }");
+  EXPECT_AVAILABLE(Header + "void fun() { o^n^e^:^:^t^w^o^:^:^e^e E; }");
+  EXPECT_AVAILABLE(Header + "void fun() { o^n^e^:^:^t^w^o:^:^c^c C; }");
+  EXPECT_UNAVAILABLE(Header +
+ "void fun() { o^n^e^:^:^t^w^o^:^:^c^c^:^:^m^m(); }");
+  EXPECT_UNAVAILABLE(Header +
+ "void fun() { o^n^e^:^:^t^w^o^:^:^c^c^:^:^s^t inst; }");
+  EXPECT_UNAVAILABLE(Header +
+ "void fun() { o^n^e^:^:^t^w^o^:^:^c^c^:^:^s^t inst; }");
+  EXPECT_UNAVAILABLE(Header + "void fun() { N^S(c^c) inst; }");
+}
+
+TEST_F(AddUsingTest, Apply) {
+  FileName = "test.cpp";
+  struct {
+llvm::StringRef TestSource;
+llvm::StringRef ExpectedSource;
+  } Cases[]{{
+// Function, no other using, namespace.
+R"cpp(
+#include "test.hpp"
+namespace {
+void fun() {
+  ^o^n^e^:^:^t^w^o^:^:^f^f();
+}
+})cpp",
+R"cpp(
+#include "test.hpp"
+namespace {using one::two::ff;
+
+void fun() {
+  ff();
+}
+})cpp",
+},
+// Type, no other using, namespace.
+{
+R"cpp(
+#include "test.hpp"
+namespace {
+void fun() {
+  ::on^e::t^wo::c^c inst;
+}
+})cpp",
+R"cpp(
+#include "test.hpp"
+namespace {using ::one::two::cc;
+
+void fun() {
+  cc inst;
+}
+})cpp",
+},
+// Type, no other using, no namespace.
+{
+R"cpp(
+#include "test.hpp"
+
+void fun() {
+  on^e::t^wo::e^e inst;
+})cpp",
+R"cpp(
+#include "test.hpp"
+
+using one::two::ee;
+
+void fun() {
+  ee inst;
+})cpp"},
+// Function, other usings.
+{
+R"cpp(
+#include "test.hpp"
+
+using one::two::cc;
+using one::two::ee;
+
+namespace {
+void fun() {
+  one::two::f^f();
+}
+})cpp",
+R"cpp(
+#include "test.hpp"
+
+using one::two::cc;
+using one::two::ff;using one::two::ee;
+
+namespace {
+void fun() {
+  ff();
+}
+})cpp",
+},
+// Function, other usings inside namespace.
+{
+R"cpp(
+#include "test.hpp"
+
+using one::two::cc;
+
+namespace {
+
+using one::two::ff;
+
+void fun() {
+  o^ne::o^o();
+}
+})cpp",
+R"cpp(
+#include "test.hpp"
+
+using one::two::cc;
+
+namespace {
+
+using one::oo;using one::two::ff;
+
+void fun() {
+  oo();
+}
+})cpp"},
+// Using comes after cursor.
+{
+R"cpp(
+#include "test.hpp"
+
+namespace {
+
+void fun() {
+  one::t^wo::ff();
+}
+
+using one::two::cc;
+
+})cpp",
+R"cpp(
+#include "test.hpp"
+
+namespace {using one::two::ff;
+
+
+void fun() {
+  ff();
+}
+
+using one::two::cc;
+
+})cpp"},
+// Pointer type.
+{R"cpp(
+#include "test.hpp"
+
+void fun() {
+  one::two::c^c *p;
+})cpp",
+ R"cpp(
+#include "test.hpp"
+
+using one::two::cc;
+
+void fun() {
+  cc *p;
+})cpp"},
+// Namespace declared via macro.
+{R"cpp(
+#include "test.hpp"
+#define NS_BEGIN(name) namespace name {
+
+NS_BEGIN(foo)
+
+void fun() {
+  one::two::f^f();
+}
+})cpp",
+ R"cpp(
+#include "test.hpp"
+#define NS_BEGIN(name) namespace name {
+
+using one::two::ff;
+
+NS_BEGIN(foo)
+
+void fun() {
+  ff();
+}
+})cpp"},
+// Inside macro argument.
+{R"cpp(
+#include "test.hpp"
+#define CALL(name) name()
+
+void fun() {
+  CALL(one::t^wo::ff);
+})cpp",
+ R"cpp(
+#include "test.hpp"
+#define CALL(name) name()
+
+using one::two::ff;
+
+void fun() {
+  CALL(ff);
+})cpp"}};
+  llvm::StringMap EditedFiles;
+  for (const auto  : Cases) {
+for (const auto  : expandCases(Case.TestSource)) {
+  ExtraFiles["test.hpp"] = R"cpp(
+namespace one {
+void oo() {}
+namespace two {
+enum ee {};
+void ff() {}
+class cc {
+public:
+  struct st { struct nested {}; };
+  static void mm() {}
+};
+}
+})cpp";
+  

[PATCH] D76432: [clangd] Add a tweak for adding "using" statement.

[Adam Czachorowski via Phabricator via cfe-commits]

adamcz updated this revision to Diff 253164.
adamcz marked 22 inline comments as done.
adamcz added a comment.

Addressed review comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76432

Files:
  clang-tools-extra/clangd/refactor/tweaks/AddUsing.cpp
  clang-tools-extra/clangd/refactor/tweaks/CMakeLists.txt
  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
@@ -2390,6 +2390,251 @@
 EXPECT_EQ(apply(Case.first), Case.second);
   }
 }
+
+TWEAK_TEST(AddUsing);
+TEST_F(AddUsingTest, Prepare) {
+  const std::string Header = R"cpp(
+#define NS(name) one::two::name
+namespace one {
+void oo() {}
+namespace two {
+enum ee {};
+void ff() {}
+class cc {
+public:
+  struct st {};
+  static void mm() {}
+};
+}
+})cpp";
+
+  EXPECT_AVAILABLE(Header + "void fun() { o^n^e^:^:^t^w^o^:^:^f^f(); }");
+  EXPECT_AVAILABLE(Header + "void fun() { o^n^e^::^o^o(); }");
+  EXPECT_AVAILABLE(Header + "void fun() { o^n^e^:^:^t^w^o^:^:^e^e E; }");
+  EXPECT_AVAILABLE(Header + "void fun() { o^n^e^:^:^t^w^o:^:^c^c C; }");
+  EXPECT_UNAVAILABLE(Header +
+ "void fun() { o^n^e^:^:^t^w^o^:^:^c^c^:^:^m^m(); }");
+  EXPECT_UNAVAILABLE(Header +
+ "void fun() { o^n^e^:^:^t^w^o^:^:^c^c^:^:^s^t inst; }");
+  EXPECT_UNAVAILABLE(Header +
+ "void fun() { o^n^e^:^:^t^w^o^:^:^c^c^:^:^s^t inst; }");
+  EXPECT_UNAVAILABLE(Header +
+ "void fun() { N^S(c^c) inst; }");
+}
+
+TEST_F(AddUsingTest, Apply) {
+  FileName = "test.cpp";
+  struct {
+llvm::StringRef TestSource;
+llvm::StringRef ExpectedSource;
+  } Cases[]{{
+// Function, no other using, namespace.
+R"cpp(
+#include "test.hpp"
+namespace {
+void fun() {
+  ^o^n^e^:^:^t^w^o^:^:^f^f();
+}
+})cpp",
+R"cpp(
+#include "test.hpp"
+namespace {using one::two::ff;
+
+void fun() {
+  ff();
+}
+})cpp",
+},
+// Type, no other using, namespace.
+{
+R"cpp(
+#include "test.hpp"
+namespace {
+void fun() {
+  ::on^e::t^wo::c^c inst;
+}
+})cpp",
+R"cpp(
+#include "test.hpp"
+namespace {using ::one::two::cc;
+
+void fun() {
+  cc inst;
+}
+})cpp",
+},
+// Type, no other using, no namespace.
+{
+R"cpp(
+#include "test.hpp"
+
+void fun() {
+  on^e::t^wo::e^e inst;
+})cpp",
+R"cpp(
+#include "test.hpp"
+
+using one::two::ee;
+
+void fun() {
+  ee inst;
+})cpp"},
+// Function, other usings.
+{
+R"cpp(
+#include "test.hpp"
+
+using one::two::cc;
+using one::two::ee;
+
+namespace {
+void fun() {
+  one::two::f^f();
+}
+})cpp",
+R"cpp(
+#include "test.hpp"
+
+using one::two::cc;
+using one::two::ff;using one::two::ee;
+
+namespace {
+void fun() {
+  ff();
+}
+})cpp",
+},
+// Function, other usings inside namespace.
+{
+R"cpp(
+#include "test.hpp"
+
+using one::two::cc;
+
+namespace {
+
+using one::two::ff;
+
+void fun() {
+  o^ne::o^o();
+}
+})cpp",
+R"cpp(
+#include "test.hpp"
+
+using one::two::cc;
+
+namespace {
+
+using one::oo;using one::two::ff;
+
+void fun() {
+  oo();
+}
+})cpp"},
+// Using comes after cursor.
+{
+R"cpp(
+#include "test.hpp"
+
+namespace {
+
+void fun() {
+  one::t^wo::ff();
+}
+
+using one::two::cc;
+
+})cpp",
+R"cpp(
+#include "test.hpp"
+
+namespace {using one::two::ff;
+
+
+void fun() {
+  ff();
+}
+
+using one::two::cc;
+
+})cpp"},
+// Pointer type.
+{R"cpp(
+#include "test.hpp"
+
+void fun() {
+  one::two::c^c *p;
+})cpp",
+ R"cpp(
+#include "test.hpp"
+
+using one::two::cc;
+
+void fun() {
+  cc *p;
+})cpp"},
+// Namespace declared via macro.
+{R"cpp(
+#include "test.hpp"
+#define NS_BEGIN(name) namespace name {
+
+NS_BEGIN(foo)
+
+void fun() {
+  one::two::f^f();
+}
+})cpp",
+ R"cpp(
+#include "test.hpp"
+#define NS_BEGIN(name) namespace name {
+
+using one::two::ff;
+
+NS_BEGIN(foo)
+
+void fun() {
+  ff();
+}
+})cpp"},
+// Inside macro argument.
+{R"cpp(
+#include "test.hpp"
+#define CALL(name) name()
+
+void fun() {
+  CALL(one::t^wo::ff);
+})cpp",
+ R"cpp(
+#include "test.hpp"
+#define CALL(name) name()
+
+using one::two::ff;
+
+void fun() {
+  CALL(ff);
+})cpp"}};
+  llvm::StringMap EditedFiles;
+  for (const auto  : Cases) {
+for (const auto  : expandCases(Case.TestSource)) {
+  ExtraFiles["test.hpp"] = R"cpp(
+namespace one {
+void oo() {}
+namespace two {
+enum ee {};
+void ff() {}
+class cc {
+public:

[PATCH] D76432: [clangd] Add a tweak for adding "using" statement.

[Adam Czachorowski via Phabricator via cfe-commits]

adamcz added inline comments.



Comment at: clang-tools-extra/clangd/refactor/tweaks/AddUsing.cpp:34
+// True iff "using" already exists and we should not add it.
+bool IdenticalUsingFound = false;
+// Location to insert the "using" statement.

sammccall wrote:
> is this redundant with Loc.isValid()?
> 
> If so, I'd either just use Loc (with a comment), or use 
> optional with a comment, to emphasize the relationship.
Yes, it's redundant. I went back and forth on this one, original version used 
Loc.isValid(). I decided I like being explicit better than having slightly more 
complicated semantics of the Loc field, but I wasn't too confident about this. 
Having second opinion helps :-)

I switched to isValid()





Comment at: clang-tools-extra/clangd/refactor/tweaks/AddUsing.cpp:37
+SourceLocation Loc;
+// Extra suffix to place after the "using" statement. Depending on what the
+// insertion point is anchored to, we may need one or more \n to ensure

sammccall wrote:
> can we get away with always inserting \n and relying on clang-format to clean 
> it up?
Nope, that doesn't work. Auto-formatting doesn't seem to remove blank lines 
between using statements, for example. It's likely to allow you grouping them 
in some way.



Comment at: clang-tools-extra/clangd/refactor/tweaks/AddUsing.cpp:44
+  // SourceLocation if the "using" statement already exists.
+  llvm::Expected
+  findInsertionPoint(const Selection );

sammccall wrote:
> this doesn't use any state - make it static or a free function?
It uses Name and NNSL . Would you prefer this as free function, with both 
passed as arguments. My initial thought was that, since prepare() and apply() 
already share these via class members this was fine, but I'm certainly not 
against making this a free function.



Comment at: clang-tools-extra/clangd/refactor/tweaks/AddUsing.cpp:56
+  return std::string(
+  llvm::formatv("Add using statement and remove full qualifier."));
+}

sammccall wrote:
> nit: not actually a statement :-(
> 
> If "using-declaration" is overly laywerly (and easily confused with 
> using-directive), then maybe we should just spell it out: `Add "using 
> foo::Bar" ...`
I think spelling it out may be too long sometimes, namespaces are often long.

Changed to using-declaration



Comment at: clang-tools-extra/clangd/refactor/tweaks/AddUsing.cpp:71
+  return true;
+}
+if (D->getDeclContext()->Encloses(SelectionDeclContext)) {

sammccall wrote:
> nit: we conventionally leave out the {} on one-line if bodies etc.
Uhh...is that a hard rule? I personally hate that, it's just asking for Apple 
SSL-style bugs
https://www.imperialviolet.org/2014/02/22/applebug.html



Comment at: clang-tools-extra/clangd/refactor/tweaks/AddUsing.cpp:89
+
+  // If we're looking at a type or NestedNameSpecifier, walk up the tree until
+  // we find the "main" node we care about, which would be ElaboratedTypeLoc or

sammccall wrote:
> I like the idea here, but I'm not sure it quite works. e.g. any typeloc has a 
> directly enclosing typeloc, the inner one can't be targeted. So what about 
> `x::S*`? (pointertypeloc > elaboratedtypeloc > recordtypeloc)?
> 
> - looping up from NNS until you get to something else makes sense
> - unwrapping typeloc -> elaboratedtypeloc makes sense, but I don't think you 
> ever want to unwrap multiple levels?
> - i don't think these cases overlap at all, you want one or the other
> 
> Am I missing something?
If you have a class foo::bar::cc and a struct st inside it, and then do:
foo::bar::c^c::st *p;
you end up with:
PointerTypeLoc -> ElaboratedTypeLoc ->NestedNameSpecifierLoc -> RecordTypeLoc
in which case you need to go up from type to NNSL and then up again, from NNSL 
to something that's not NNSL.

You have a good point with the PointerTypeLoc, that's a bug. We should stop 
going up the tree as soon as we find ElaboratedTypeLoc. I added a test for that.



Comment at: clang-tools-extra/clangd/refactor/tweaks/AddUsing.cpp:132
+llvm::Expected
+AddUsing::findInsertionPoint(const Selection ) {
+  auto  = Inputs.AST->getSourceManager();

sammccall wrote:
> One possible hiccup is insertion splitting a comment from its target. Cases I 
> can think of:
> - `namespace { // anonymous` -> `namespace { using foo::bar; // anonymous`. 
> This seems rare/unimportant.
> - `/* docs */ int firstTopLevelDecl` -> `/* docs */ using foo::bar; int 
> firstTopLevelDecl`. This seems common/bad.
> - `/* for widgets */using foo::Qux;` -> `/* for widgets */ using foo::bar; 
> using foo::Qux`. This seems rare/unimportant.
> 
> I think you could handle the decl case by calling 
> `ASTContext::getLocalCommentForDeclUncached()` and using the getBeginLoc() of 
> the returned comment if any.
> 
> That said, 

[PATCH] D76432: [clangd] Add a tweak for adding "using" statement.

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

This seems really well-thought-out.
I'm being (even) more verbose than usual about the interesting AST details. 
Please do push back/defer fixing anything that adds a lot of complexity and 
doesn't seem important.




Comment at: clang-tools-extra/clangd/refactor/tweaks/AddUsing.cpp:20
+
+// Tweak for removing full namespace qualifier under cursor on function calls
+// and types and adding "using" statement instead.

function calls is obsolete I think, I'd just call these references or 
DeclRefExpr



Comment at: clang-tools-extra/clangd/refactor/tweaks/AddUsing.cpp:22
+// and types and adding "using" statement instead.
+class AddUsing : public Tweak {
+public:

You could put some general comments about the scope/choices here (applicable 
only to namespaces, insertion strategy)



Comment at: clang-tools-extra/clangd/refactor/tweaks/AddUsing.cpp:22
+// and types and adding "using" statement instead.
+class AddUsing : public Tweak {
+public:

sammccall wrote:
> You could put some general comments about the scope/choices here (applicable 
> only to namespaces, insertion strategy)
> Removing qualifier from other instances of the same type/call.

I think this is valuable enough to be worth calling out in the top-level 
comment as not-yet-done.
The other extensions you mention seem like we might never get around to them 
and it'd be fine.



Comment at: clang-tools-extra/clangd/refactor/tweaks/AddUsing.cpp:34
+// True iff "using" already exists and we should not add it.
+bool IdenticalUsingFound = false;
+// Location to insert the "using" statement.

is this redundant with Loc.isValid()?

If so, I'd either just use Loc (with a comment), or use 
optional with a comment, to emphasize the relationship.



Comment at: clang-tools-extra/clangd/refactor/tweaks/AddUsing.cpp:37
+SourceLocation Loc;
+// Extra suffix to place after the "using" statement. Depending on what the
+// insertion point is anchored to, we may need one or more \n to ensure

can we get away with always inserting \n and relying on clang-format to clean 
it up?



Comment at: clang-tools-extra/clangd/refactor/tweaks/AddUsing.cpp:44
+  // SourceLocation if the "using" statement already exists.
+  llvm::Expected
+  findInsertionPoint(const Selection );

this doesn't use any state - make it static or a free function?



Comment at: clang-tools-extra/clangd/refactor/tweaks/AddUsing.cpp:48
+  // The qualifier to remove. Set by prepare().
+  NestedNameSpecifierLoc NNSL;
+  // The name following NNSL. Set by prepare().

nit: I quite like the abbreviated names for locals but this deserves a real 
name like Qualifier or QualifierToRemove.



Comment at: clang-tools-extra/clangd/refactor/tweaks/AddUsing.cpp:54
+
+std::string AddUsing::title() const {
+  return std::string(

Can we make this dynamic? The more clearly we communicate what is targeted, the 
more confidence the user will have to use it.

`title()` may only be called after a successful prepare(), so we can use the 
prepared state.

Maybe something like `Add using-declaration for Bar, and remove qualifier`?



Comment at: clang-tools-extra/clangd/refactor/tweaks/AddUsing.cpp:56
+  return std::string(
+  llvm::formatv("Add using statement and remove full qualifier."));
+}

nit: not actually a statement :-(

If "using-declaration" is overly laywerly (and easily confused with 
using-directive), then maybe we should just spell it out: `Add "using foo::Bar" 
...`



Comment at: clang-tools-extra/clangd/refactor/tweaks/AddUsing.cpp:68
+auto Loc = D->getUsingLoc();
+if (Loc.isInvalid() || Loc.isMacroID() ||
+SM.getFileID(Loc) != SM.getMainFileID()) {

no need for the first two conditions, getFileID will give you an invalid fileid 
and a macro fileid respectively



Comment at: clang-tools-extra/clangd/refactor/tweaks/AddUsing.cpp:71
+  return true;
+}
+if (D->getDeclContext()->Encloses(SelectionDeclContext)) {

nit: we conventionally leave out the {} on one-line if bodies etc.



Comment at: clang-tools-extra/clangd/refactor/tweaks/AddUsing.cpp:72
+}
+if (D->getDeclContext()->Encloses(SelectionDeclContext)) {
+  Results.push_back(D);

This check is sufficient for correctness, but we're still going to traverse all 
the parts of the AST that can't pass this check.

I think you can override TraverseDecl. If the Decl is a DeclContext and it 
doesn't enclose the selectiondc, then return (pruning that subtree). Otherwise 
call RecursiveASTVisitor::TraverseDecl and continue as before.

(I think you still want this check though - pruning 

[PATCH] D76432: [clangd] Add a tweak for adding "using" statement.

[Adam Czachorowski via Phabricator via cfe-commits]

adamcz created this revision.
Herald added subscribers: cfe-commits, usaxena95, kadircet, arphaman, mgrang, 
jkorous, MaskRay, ilya-biryukov, mgorny.
Herald added a project: clang.

This triggers on types and function calls with namespace qualifiers. The
action is to remove the qualifier and instead add a "using" statement at
appropriate place.

It is not always clear where to add the "using" line. Right now we find
the nearest "using" line and add it there, thus keeping with local
convention. If there are no usings, we put it at the deepest relevant
namespace level.

This is an initial version only. There are several improvements that
can be made:

- Support for qualifiers that are not purely namespace (e.g.  record

types, etc).

- Removing qualifier from other instances of the same type/call.
- Smarter placement of the "using" line.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D76432

Files:
  clang-tools-extra/clangd/refactor/tweaks/AddUsing.cpp
  clang-tools-extra/clangd/refactor/tweaks/CMakeLists.txt
  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
@@ -2390,6 +2390,191 @@
 EXPECT_EQ(apply(Case.first), Case.second);
   }
 }
+
+TWEAK_TEST(AddUsing);
+TEST_F(AddUsingTest, Prepare) {
+  const std::string Header = R"cpp(
+namespace one {
+void oo() {}
+namespace two {
+enum ee {};
+void ff() {}
+class cc {
+public:
+  struct st {};
+  static void mm() {}
+};
+}
+})cpp";
+
+  EXPECT_AVAILABLE(Header + "void fun() { o^n^e^:^:^t^w^o^:^:^f^f(); }");
+  EXPECT_AVAILABLE(Header + "void fun() { o^n^e^::^o^o(); }");
+  EXPECT_AVAILABLE(Header + "void fun() { o^n^e^:^:^t^w^o^:^:^e^e E; }");
+  EXPECT_AVAILABLE(Header + "void fun() { o^n^e^:^:^t^w^o:^:^c^c C; }");
+  EXPECT_UNAVAILABLE(Header +
+ "void fun() { o^n^e^:^:^t^w^o^:^:^c^c^:^:^m^m(); }");
+  EXPECT_UNAVAILABLE(Header +
+ "void fun() { o^n^e^:^:^t^w^o^:^:^c^c^:^:^s^t inst; }");
+}
+
+TEST_F(AddUsingTest, Apply) {
+  FileName = "test.cpp";
+  struct {
+llvm::StringRef TestSource;
+llvm::StringRef ExpectedSource;
+  } Cases[]{{
+// Function, no other using, namespace.
+R"cpp(
+#include "test.hpp"
+namespace {
+void fun() {
+  ^o^n^e^:^:^t^w^o^:^:^f^f();
+}
+})cpp",
+R"cpp(
+#include "test.hpp"
+namespace {using one::two::ff;
+
+void fun() {
+  ff();
+}
+})cpp",
+},
+// Type, no other using, namespace.
+{
+R"cpp(
+#include "test.hpp"
+namespace {
+void fun() {
+  on^e::t^wo::c^c inst;
+}
+})cpp",
+R"cpp(
+#include "test.hpp"
+namespace {using one::two::cc;
+
+void fun() {
+  cc inst;
+}
+})cpp",
+},
+// Type, no other using, no namespace.
+{
+R"cpp(
+#include "test.hpp"
+
+void fun() {
+  on^e::t^wo::e^e inst;
+})cpp",
+R"cpp(
+#include "test.hpp"
+
+using one::two::ee;
+
+void fun() {
+  ee inst;
+})cpp"},
+// Function, other usings.
+{
+R"cpp(
+#include "test.hpp"
+
+using one::two::cc;
+using one::two::ee;
+
+namespace {
+void fun() {
+  one::two::f^f();
+}
+})cpp",
+R"cpp(
+#include "test.hpp"
+
+using one::two::cc;
+using one::two::ff;using one::two::ee;
+
+namespace {
+void fun() {
+  ff();
+}
+})cpp",
+},
+// Function, other usings inside namespace.
+{
+R"cpp(
+#include "test.hpp"
+
+using one::two::cc;
+
+namespace {
+
+using one::two::ff;
+
+void fun() {
+  o^ne::o^o();
+}
+})cpp",
+R"cpp(
+#include "test.hpp"
+
+using one::two::cc;
+
+namespace {
+
+using one::oo;using one::two::ff;
+
+void fun() {
+  oo();
+}
+})cpp"},
+// Using comes after cursor.
+{
+R"cpp(
+#include "test.hpp"
+
+namespace {
+
+void fun() {
+  one::t^wo::ff();
+}
+
+using one::two::cc;
+
+})cpp",
+R"cpp(
+#include "test.hpp"
+
+namespace {using one::two::ff;
+
+
+void fun() {
+  ff();
+}
+
+using one::two::cc;
+
+})cpp"}};
+  llvm::StringMap EditedFiles;
+  for (const auto  : Cases) {
+for (const auto  : expandCases(Case.TestSource)) {
+  ExtraFiles["test.hpp"] = R"cpp(
+namespace one {
+void oo() {}
+namespace two {
+enum ee {};
+void ff() {}
+class cc {
+public:
+  struct st {};
+  static void mm() {}
+};
+}
+})cpp";
+  EXPECT_EQ(apply(SubCase, ), Case.ExpectedSource);
+}
+  }
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clang-tools-extra/clangd/refactor/tweaks/CMakeLists.txt
===
--- clang-tools-extra/clangd/refactor/tweaks/CMakeLists.txt
+++ clang-tools-extra/clangd/refactor/tweaks/CMakeLists.txt
@@ -12,6