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

[v1nh1shungry via Phabricator via cfe-commits]

v1nh1shungry added a comment.

Oops! Thank you for your thoughtful consideration.

You're right, but I currently get stuck there and need more time. And I prefer to clear the existing patches simultaneously if you don't mind.

Or do you think I should merge the modification of this patch into there and give up this?



================
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) {
----------------
v1nh1shungry wrote:
> 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.
FYI, we have a discussion left here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138028