[PATCH] D138028: [clangd] Fix action `RemoveUsingNamespace` for inline namespace

[v1nh1shungry via Phabricator via cfe-commits]

v1nh1shungry added inline comments.


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/RemoveUsingNamespace.cpp:212
   // Produce replacements to add the qualifiers.
   std::string Qualifier = printUsingNamespaceName(Ctx, *TargetDirective) + "::";
   for (auto Loc : IdentsToQualify) {
----------------
tom-anders wrote:
> v1nh1shungry wrote:
> > We can replace `printUsingNamespaceName` with `printNamespaceScope` here so that we can get `a::foobar()` in the test. 
> > 
> > However, it can sometimes cause redundancy such as in the 10th test. 
> > 
> > And I don't know whether it is worth it. WDYT?
> Just making sure I understood this correctly:
> 
> If you replace `printUsingNamespaceName` with `printNamespaceScope`, then...
> 
> - ...in the test you added it would result in `a::foobar()` instead of `a::b::foobar()` (which is better)
> - ... but in this test (which is the 10th test if I counted correctly):
>      
> ```
>  namespace a::b { struct Foo {}; }
>   using namespace a;
>   using namespace a::[[b]];
>   using namespace b;
>   int main() { Foo F;}
> ```
> what would be the result..? would you get `a::Foo` instead of `a::b::Foo`?
> 
Sorry, I mean the next test. I read `10` from the inlay hint but I forgot the index starts from `0` :(

The test I want to mention:
```
namespace a::b { struct Foo {}; }
using namespace a;
using namespace a::b;
using namespace [[b]];
int main() { Foo F;}
```

We will get `a::b::Foo` in both the 10th and 11th tests. So in the 10th test, we don't get any benefits and don't sacrifice anything. In the 11th test, we get more redundancy than the existing version.

Apologize again for my mistake.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138028