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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
