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

[v1nh1shungry via Phabricator via cfe-commits]

v1nh1shungry added a comment.

@kadircet Thank you for the excellent suggestions! Will dive into it to figure out the implementations.



================
Comment at: clang-tools-extra/clangd/unittests/tweaks/RemoveUsingNamespaceTests.cpp:294
+}
+using namespace ns1::ns2;
+using namespace ns1::ns3;
----------------
kadircet wrote:
> 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).
You're right. Sorry for misunderstanding it.


================
Comment at: clang-tools-extra/clangd/unittests/tweaks/RemoveUsingNamespaceTests.cpp:294
+}
+using namespace ns1::ns2;
+using namespace ns1::ns3;
----------------
v1nh1shungry wrote:
> kadircet wrote:
> > 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).
> You're right. Sorry for misunderstanding it.
Yes, I agree.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137817