ilya-biryukov added a comment.
The main comment is about limiting this only to global namespace. PTAL.
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/RemoveUsingNamespace.cpp:79
+}
+} // namespace
+
----------------
continue rest of the file in the anonymous namespace
================
Comment at:
clang-tools-extra/clangd/refactor/tweaks/RemoveUsingNamespace.cpp:110
+
+ // FIXME: if this is not coming from a macro argument, remove.
+ for (auto *T : Ref.Targets) {
----------------
I think I had this FIXME in some of the versions, but TBF I don't remember why
I added it.
Maybe remove?
================
Comment at:
clang-tools-extra/clangd/refactor/tweaks/RemoveUsingNamespace.cpp:146
+ for (auto Loc : IdentsToQualify) {
+ if (auto Err = R.add(tooling::Replacement(Ctx.getSourceManager(), Loc,
+ /*Length=*/0, Qualifier)))
----------------
Qualifying identifiers and removing using namespaces might actually conflict.
Could you add a (commented-out) test with an example that fails and a FIXME?
```
namespace a::b { struct foo {}; }
using namespace a::b;
using namespace b; // we'll try to both qualify this 'b' and remove this line.
```
================
Comment at: clang-tools-extra/clangd/unittests/TweakTests.cpp:774
+ )cpp"},
+ {// FIXME: Unable to qualify ident from inner ns.
+ R"cpp(
----------------
Could we disallow the action outside global namespace for now to make sure it's
always correct.
```
#include <vector>
using namespace std;
```
is a very common pattern anyway, supporting only it in the first version looks
ok.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D68562/new/
https://reviews.llvm.org/D68562
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
