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