[PATCH] D68562: [clangd] Add RemoveUsingNamespace tweak.

[Ilya Biryukov via Phabricator via cfe-commits]

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