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

[Tom Praschan via Phabricator via cfe-commits]

tom-anders added inline comments.


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/RemoveUsingNamespace.cpp:93
+// Return true if `LHS` is declared in `RHS`
+bool declareIn(const NamedDecl *LHS, const DeclContext *RHS) {
+  const auto *D = LHS->getDeclContext();
----------------



================
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:
> 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.
Ok I’d say let’s just go with printUsingNamespaceName here for now. When your other patch is merged as well, maybe we could have a look at this again.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138028