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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/RemoveUsingNamespace.cpp:86
+  if (const Decl *ParentDecl = Node->Parent->ASTNode.get<Decl>()) {
+    return llvm::isa<TranslationUnitDecl>(ParentDecl);
+  }
----------------
NIT: remove redundant `{}`  around this `return`


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/RemoveUsingNamespace.cpp:99
+    return false;
+  if (!dyn_cast<Decl>(TargetDirective->getDeclContext()))
+    return false;
----------------
I believe this check is redundant in presence of `isTopLevelDecl()`...
(It used to check the 'using namespace' is not inside a function body)


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/RemoveUsingNamespace.cpp:121
+      for (auto *T : Ref.Targets) {
+        // FIXME: handle inline namespaces, unscoped enumerators.
+        if (T->getDeclContext() != TargetDirective->getNominatedNamespace())
----------------
Could you take a look at handling these?

Also happy if we make it a separate change, but we shouldn't delay this.
Both are important cases.

The examples should roughly be:
```
namespace std {
inline namespace v1 {
  struct vector {};
}
}

using namespace std;
vector v;
```

and
```
namespace tokens {
enum Token {
  comma, identifier, numeric
};
}

using namespace tokens;
auto x = comma; 
```


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/RemoveUsingNamespace.cpp:163
+std::string RemoveUsingNamespace::title() const {
+  return llvm::formatv("Remove using namespace, add qualifiers instead");
+}
----------------
NIT: could you rephrase as `re-qualify names instead`
"add qualifiers" confuses some people, this can be read as "add a const qualifier"


================
Comment at: clang-tools-extra/clangd/unittests/TweakTests.cpp:781
+        namespace bb { struct map {}; }
+        using namespace bb; // Qualifies this.
+      }
----------------
Argh, this should definitely be fixed :-(
One simple way to handle this particular only qualify references, which come **after** the first `using namespace` we are removing.

There's a `SourceManager::isBeforeInTranslationUnit` function that can be used to find out whether something is written before or after a particular point.

This won't fix all of the cases, but fixing in the general case seems hard.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68562