ilya-biryukov added a comment. The most important comment is in the tests. Is there a way to have the same effect with less changes?
================ Comment at: clang-tools-extra/clangd/refactor/tweaks/RemoveUsingNamespace.cpp:47 const UsingDirectiveDecl *TargetDirective = nullptr; + const NamespaceDecl *ContainingNS = nullptr; }; ---------------- NIT: could you add a short comment mentioning this is exactly the `DeclContext` of `TargetDirective`? ================ Comment at: clang-tools-extra/clangd/refactor/tweaks/RemoveUsingNamespace.cpp:110 + auto DeclCtx = TargetDirective->getDeclContext(); + if (!dyn_cast<Decl>(DeclCtx)) return false; ---------------- This check looks redundant now that we check the decl context is a namespace of a translation unit. I think it was originally intended to check the whether the `using namespace` is inside a function or not. ================ Comment at: clang-tools-extra/clangd/refactor/tweaks/RemoveUsingNamespace.cpp:173 + + CharSourceRange RemoveQualifier; + RemoveQualifier.setBegin(Ref.Qualifier ? Ref.Qualifier.getBeginLoc() : Loc); ---------------- NIT: maybe simplify to ``` QualifierReplacements.push_back( CharSourceRange::getTokenRange( Ref.Qualifier ? Ref.Qualifier.getBeginLoc : Loc, Loc)); ``` ? ================ Comment at: clang-tools-extra/clangd/refactor/tweaks/RemoveUsingNamespace.cpp:180 + if (ContainingNS) + findExplicitReferences(ContainingNS, SelectRefToQualify); + else ---------------- Do we also need to run in the redeclarations of the same namespace? ``` namespace clangd { using namespace clang; } namespace clangd { /*clang::*/Decl* D = nullptr; } ``` ================ Comment at: clang-tools-extra/clangd/refactor/tweaks/RemoveUsingNamespace.cpp:182 + else + for (auto &D : Inputs.AST.getLocalTopLevelDecls()) + findExplicitReferences(D, SelectRefToQualify); ---------------- NIT: could you add braces around the branches of the if statement? for loop is complicated, so it probably qualifies as something that asks for the braces. ================ Comment at: clang-tools-extra/clangd/unittests/TweakTests.cpp:898 + A::B::vector V3; + A::B::std::vector V4; + ::A::B::std::vector V5; ---------------- Is there a way to avoid re-qualifying this? We should aim for minimal changes and there is probably a simple way to achieve this by only qualifying when the qualifier itself references the `ContainingNS` and target is inside the target namespace of the directive being removed. Or this too complicated for some reason? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69162/new/ https://reviews.llvm.org/D69162 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits