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