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