[PATCH] D137817: [clangd] Improve action `RemoveUsingNamespace` for user-defined literals

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.


================
Comment at: clang-tools-extra/clangd/unittests/tweaks/RemoveUsingNamespaceTests.cpp:294
+}
+using namespace ns1::ns2;
+using namespace ns1::ns3;
----------------
v1nh1shungry wrote:
> kadircet wrote:
> > sorry i am having trouble understanding why we are:
> > - only handling user defined literals from inline namespaces and not others?
> > - producing using directives and not using declarations
> > - inserting these at top level rather than where the usage is
> The first question:
> 
> Because others have already been handled. Say
> 
> ```
> namespace a { inline namespace b { void foobar(); } }
> using namespace ^a;
> int main() { foobar(); }
> ```
> 
> can become
> 
> ```
> namespace a { inline namespace b { void foobar(); } }
> 
> int main() { a::foobar(); }
> ```
> 
> But user-defined literals just can't add a qualifier, right?
> 
> ---
> 
> The second question:
> 
> Yes, this is a good idea. I didn't realize we can use using-declarations instead of using-directives.
> 
> ---
> 
> The last question:
> 
> Hmm, I think it is cleaner if there are multiple usages. Since we can only remove the using-directives at the top level, this doesn't hurt anything. And it is the easiest solution to implement as well :)
> Because others have already been handled. Say

i was emphasizing on the difference between user defined literals inside inline namespaces, and user defined literals from regular namespaces. not other types of decls inside an inline namespace, eg:
```
namespace ns { long double operator"" _o(long double); }
```

we should also introduce a using-decl for `_o` despite it not being inside an inline namespace.


================
Comment at: clang-tools-extra/clangd/unittests/tweaks/RemoveUsingNamespaceTests.cpp:294
+}
+using namespace ns1::ns2;
+using namespace ns1::ns3;
----------------
kadircet wrote:
> v1nh1shungry wrote:
> > kadircet wrote:
> > > sorry i am having trouble understanding why we are:
> > > - only handling user defined literals from inline namespaces and not others?
> > > - producing using directives and not using declarations
> > > - inserting these at top level rather than where the usage is
> > The first question:
> > 
> > Because others have already been handled. Say
> > 
> > ```
> > namespace a { inline namespace b { void foobar(); } }
> > using namespace ^a;
> > int main() { foobar(); }
> > ```
> > 
> > can become
> > 
> > ```
> > namespace a { inline namespace b { void foobar(); } }
> > 
> > int main() { a::foobar(); }
> > ```
> > 
> > But user-defined literals just can't add a qualifier, right?
> > 
> > ---
> > 
> > The second question:
> > 
> > Yes, this is a good idea. I didn't realize we can use using-declarations instead of using-directives.
> > 
> > ---
> > 
> > The last question:
> > 
> > Hmm, I think it is cleaner if there are multiple usages. Since we can only remove the using-directives at the top level, this doesn't hurt anything. And it is the easiest solution to implement as well :)
> > Because others have already been handled. Say
> 
> i was emphasizing on the difference between user defined literals inside inline namespaces, and user defined literals from regular namespaces. not other types of decls inside an inline namespace, eg:
> ```
> namespace ns { long double operator"" _o(long double); }
> ```
> 
> we should also introduce a using-decl for `_o` despite it not being inside an inline namespace.
> Hmm, I think it is cleaner if there are multiple usages. Since we can only remove the using-directives at the top level, this doesn't hurt anything. And it is the easiest solution to implement as well :)

right, but introducing these at the top level will have unintended consequences (e.g. if this is a header, symbols will all of a sudden be visible in all the dependents).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137817