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

[Ilya Biryukov via Phabricator via cfe-commits]

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