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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov accepted this revision.
ilya-biryukov added a comment.
This revision is now accepted and ready to land.

LGTM, many thanks!

A few last NITs too



================
Comment at: clang-tools-extra/clangd/refactor/tweaks/RemoveUsingNamespace.cpp:31
+///   std::vector<int> foo(std::map<int, int>);
+// Currently limited to using namespace directives inside global namespace to
+// simplify implementation. Also the namespace must not contain a using
----------------
NIT: use three slashes


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/RemoveUsingNamespace.cpp:32
+// Currently limited to using namespace directives inside global namespace to
+// simplify implementation. Also the namespace must not contain a using
+// directives.
----------------
NIT: s/a using directives/using directives


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/RemoveUsingNamespace.cpp:91
+
+// Returns the first visible namespace decl that contains this DeclContext
+// For example: Returns ns1 for S1 and a.
----------------
NIT: end the sentence with a period


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/RemoveUsingNamespace.cpp:91
+
+// Returns the first visible namespace decl that contains this DeclContext
+// For example: Returns ns1 for S1 and a.
----------------
ilya-biryukov wrote:
> NIT: end the sentence with a period
I believe this function is more general, e.g. it returns the parent class for class members.
I suggest renaming to `visibleContext` and removing mentions of returning a namespace in the doc comment.


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/RemoveUsingNamespace.cpp:99
+  while (D->isInlineNamespace() || D->isTransparentContext()) {
+    D = D->getParent();
+  }
----------------
NIT: remove braces, LLVM code style suggests to avoid them in simple single-statement loops


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/RemoveUsingNamespace.cpp:116
+  // It is non-trivial to deal with cases where identifiers come from the inner
+  // namepsace. For example map has to be changed to aa::map.
+  // namespace aa {
----------------
NIT:s/namepsace/namespace


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/RemoveUsingNamespace.cpp:137
+
+  // Location of first UsingDirective in the file.
+  SourceLocation FirstUsingDirectiveLoc =
----------------
NIT: the comment looks redundant, the variable name is very descriptive. Maybe remove?


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/RemoveUsingNamespace.cpp:139
+  SourceLocation FirstUsingDirectiveLoc =
+      SM.getLocForEndOfFile(SM.getMainFileID());
+  for (auto *D : AllDirectives) {
----------------
I'm not 100% certain this is considered to be the end location, as there macros, etc.
Could we instead start with an invalid source location


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/RemoveUsingNamespace.cpp:162
+        // incorrect, e.g.
+        // namespace std { int foo(); }
+        // #define FOO 1 + foo()
----------------
NIT: could you indent the example?
To make it clearly distinguishable from the rest of the comment


================
Comment at: clang-tools-extra/clangd/unittests/TweakTests.cpp:810
+      using namespace a::[[b]];
+      using namespace b;
+      int main() { Foo F;}
----------------
What happens if we remove this one?
Will it still qualify as `a::b` or as `b::`?

Could we add a test just to document the behavior?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68562