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