[PATCH] D91966: [clangd] AddUsing: Used spelled text instead of type name.

[Kadir Cetinkaya via Phabricator via cfe-commits]

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

thanks, lgtm!



================
Comment at: clang-tools-extra/clangd/refactor/tweaks/AddUsing.cpp:268
+                                               SpelledTokens->back());
+      llvm::StringRef NameRef = SpelledRange.text(SM);
+
----------------
adamcz wrote:
> kadircet wrote:
> > what about saving the full range here, and using it's `start` and `length - name.size()` as removal range in `apply`?
> > 
> > similarly we can also use the `range.text` as text to insert. that way we can inline `getNNSLAsString` as it won't be needed elsewhere anymore.
> We only have a range in this branch of the code. The other one (for DeclRefExpr) is not using TokenBuffers, so no range right now.
> 
> We may change it in the future. Or would you prefer this changed now?
> We only have a range in this branch of the code. The other one (for DeclRefExpr) is not using TokenBuffers, so no range right now.

Ah right, I've forgot that one. My main concern was the extra `getSpellingLoc` in `apply`.

> We may change it in the future. Or would you prefer this changed now?

No hurries.


================
Comment at: clang-tools-extra/clangd/unittests/TweakTests.cpp:2814
+  uu u;
 })cpp"}};
   llvm::StringMap<std::string> EditedFiles;
----------------
adamcz wrote:
> kadircet wrote:
> > Tests don't seem to be covering something like:
> > ```
> > namespace foo { namespace bar { struct X {}; } }
> > using namespace foo;
> > bar::X^ x;
> > ```
> So this has the unfortunate side-effect of triggering a variation of https://github.com/clangd/clangd/issues/585
> 
> That's something I'm fixing in a later change (although this case is still not handled). For now, I'm putting an extra namespace scope to make this test correct, while still testing your example.
sorry for being confusing, I was actually suggesting putting:
```
namespace foo { namespace bar { struct X {}; } }
using namespace foo;
```
into the header (to prevent that bug from triggering), and only having the usage in the CC file. but this works too.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91966