ilya-biryukov added a comment.
Many thanks, this LG overall, just a few NITs and documentation requests.
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/RemoveUsingNamespace.cpp:31
+/// std::vector<int> foo(std::map<int, int>);
+class RemoveUsingNamespace : public Tweak {
+public:
----------------
Could you mention current limitation in the comment?
Something like
```
Currently limited to using namespace directives inside global namespace to
simplify implementation.
```
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/RemoveUsingNamespace.cpp:85
+bool isTopLevelDecl(const SelectionTree::Node *Node) {
+ if (!Node->Parent)
+ return false;
----------------
NIT: could be written as a single statement
```
return Node->Parent
&& Node->Parent->....
```
up to you, though, both versions look fine.
================
Comment at:
clang-tools-extra/clangd/refactor/tweaks/RemoveUsingNamespace.cpp:113
+ return false;
+ // FIXME: Unavailable for namespaces containing using-namespace decl.
+ if (!TargetDirective->getNominatedNamespace()->using_directives().empty())
----------------
Could you provide the rationale why we do this too?
This information would be super-useful to folks fixing this later
================
Comment at:
clang-tools-extra/clangd/refactor/tweaks/RemoveUsingNamespace.cpp:150
+ if (Loc.isMacroID()) {
+ if (!SM.isMacroArgExpansion(Loc))
+ return; // FIXME: report a warning to the users.
----------------
Worth documenting why we do this here:
```
Avoid adding qualifiers before macro expansions, it's probably incorrect, e.g.
#define FOO 1 + matrix()
using namespace linalg; // provides matrix
auto x = FOO;
Should not be changed to:
auto x = linalg::FOO;
```
================
Comment at:
clang-tools-extra/clangd/refactor/tweaks/RemoveUsingNamespace.cpp:157
+ return; // FIXME: report these to the user as warnings?
+ if (SM.isBeforeInTranslationUnit(Loc, FirstUsingDirectiveLoc))
+ return;
----------------
NIT: could you add a comment on why we do this? Something like
```
Directive was not visible before this point.
```
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
