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

[Adam Czachorowski via Phabricator via cfe-commits]

adamcz added inline comments.


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/AddUsing.cpp:268
+                                               SpelledTokens->back());
+      llvm::StringRef NameRef = SpelledRange.text(SM);
+
----------------
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?


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/AddUsing.cpp:274
+        return false; // What's spelled doesn't match the qualifier.
+      Name = NameRef.str();
     }
----------------
kadircet wrote:
> it is not that important, but I suppose `NameRef` is going to be alive during apply, so I suppose we can keep `Name` as a `llvm::StringRef`.
Right, it used to be NameRef, then in some random version of the code pre-review it had to be a string and I never reverted that. Good point, thanks.


================
Comment at: clang-tools-extra/clangd/unittests/TweakTests.cpp:2814
+  uu u;
 })cpp"}};
   llvm::StringMap<std::string> EditedFiles;
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91966