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