usaxena95 updated this revision to Diff 225596.
usaxena95 added a comment.
Added additional tests.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D69162/new/
https://reviews.llvm.org/D69162
Files:
clang-tools-extra/clangd/refactor/tweaks/RemoveUsingNamespace.cpp
clang-tools-extra/clangd/unittests/TweakTests.cpp
Index: clang-tools-extra/clangd/unittests/TweakTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/TweakTests.cpp
+++ clang-tools-extra/clangd/unittests/TweakTests.cpp
@@ -789,15 +789,23 @@
map m;
}
)cpp"},
- {// Available only for top level namespace decl.
- R"cpp(
- namespace aa {
- namespace bb { struct map {}; }
- using namespace b^b;
- }
- int main() { aa::map m; }
+ {
+ // FIXME: Does not remove usages outside the enclosing NS.
+ R"cpp(
+ namespace aa {
+ namespace bb { struct map {}; }
+ using namespace b^b;
+ }
+ int main() { aa::map m; }
)cpp",
- "unavailable"},
+ R"cpp(
+ namespace aa {
+ namespace bb { struct map {}; }
+
+ }
+ int main() { aa::map m; }
+ )cpp",
+ },
{// FIXME: Unavailable for namespaces containing using-namespace decl.
R"cpp(
namespace aa {
@@ -875,7 +883,116 @@
int main() {
std::vector V;
}
- )cpp"}};
+ )cpp"},
+ {// using inner namespaces.
+ R"cpp(
+ namespace A { namespace B {
+ inline namespace ns1 { namespace std { struct vector {}; } }
+ using namespace st^d;
+ using namespace A::B::std;
+ using namespace B::std;
+ void func() {
+ vector V1;
+ B::vector V2;
+ A::B::vector V3;
+ A::B::std::vector V4;
+ ::A::B::std::vector V5;
+ ::A::B::ns1::std::vector V6;
+ }
+ }}
+ )cpp",
+ R"cpp(
+ namespace A { namespace B {
+ inline namespace ns1 { namespace std { struct vector {}; } }
+
+
+
+ void func() {
+ std::vector V1;
+ std::vector V2;
+ std::vector V3;
+ std::vector V4;
+ std::vector V5;
+ std::vector V6;
+ }
+ }}
+ )cpp"},
+ {// using outer namespaces.
+ R"cpp(
+ namespace std { struct vector {}; }
+ namespace A { namespace B {
+ using namespace st^d;
+ void func() {
+ vector V1;
+ B::vector V2;
+ A::B::vector V3;
+ ::A::B::vector V4;
+ }
+ }}
+ )cpp",
+ R"cpp(
+ namespace std { struct vector {}; }
+ namespace A { namespace B {
+
+ void func() {
+ std::vector V1;
+ std::vector V2;
+ std::vector V3;
+ std::vector V4;
+ }
+ }}
+ )cpp"},
+ {// FIXME: No replacements produced for extended namespace.
+ R"cpp(
+ namespace std { struct vector {}; }
+ namespace A { namespace B { using namespace st^d; }}
+ namespace A { namespace B { int func() { vector V1;}}}
+ )cpp",
+ R"cpp(
+ namespace std { struct vector {}; }
+ namespace A { namespace B { }}
+ namespace A { namespace B { int func() { vector V1;}}}
+ )cpp"},
+ {// using namespace for outer and inner namespaces: remove inner.
+ R"cpp(
+ namespace out { namespace std { struct vector{}; }}
+ namespace A {
+ using namespace out;
+ using namespace st^d;
+ void func() { vector V;}
+ }
+ )cpp",
+ R"cpp(
+ namespace out { namespace std { struct vector{}; }}
+ namespace A {
+ using namespace out;
+
+ void func() { std::vector V;}
+ }
+ )cpp"},
+ {// using namespace for outer and inner namespaces: remove outer.
+ R"cpp(
+ namespace out { namespace std { struct vector{}; }}
+ namespace A {
+ using namespace ou^t;
+ using namespace std;
+ void func() { vector V;}
+ }
+ )cpp",
+ R"cpp(
+ namespace out { namespace std { struct vector{}; }}
+ namespace A {
+
+ using namespace out::std;
+ void func() { vector V;}
+ }
+ )cpp"},
+ {// Not available for contexts that are not global or namespaces.
+ R"cpp(
+ namespace std { struct vector {}; }
+ int main() { if(true) { using namespace st^d;}}
+ )cpp",
+ "unavailable"}};
for (auto C : Cases)
EXPECT_EQ(C.second, apply(C.first)) << C.first;
}
Index: clang-tools-extra/clangd/refactor/tweaks/RemoveUsingNamespace.cpp
===================================================================
--- clang-tools-extra/clangd/refactor/tweaks/RemoveUsingNamespace.cpp
+++ clang-tools-extra/clangd/refactor/tweaks/RemoveUsingNamespace.cpp
@@ -18,6 +18,7 @@
#include "clang/Tooling/Core/Replacement.h"
#include "clang/Tooling/Refactoring/RecursiveSymbolVisitor.h"
#include "llvm/ADT/ScopeExit.h"
+#include <vector>
namespace clang {
namespace clangd {
@@ -28,9 +29,10 @@
/// vector<int> foo(std::map<int, int>);
/// Would become:
/// std::vector<int> foo(std::map<int, int>);
-/// Currently limited to using namespace directives inside global namespace to
-/// simplify implementation. Also the namespace must not contain using
-/// directives.
+///
+/// Currently limited to using namespace directive that is global or inside a
+/// namespace. Also the namespace must not contain any using directives.
+/// Qualifies all the references that are inside the enclosing namespace.
class RemoveUsingNamespace : public Tweak {
public:
const char *id() const override;
@@ -42,6 +44,7 @@
private:
const UsingDirectiveDecl *TargetDirective = nullptr;
+ const NamespaceDecl *ContainingNS = nullptr;
};
REGISTER_TWEAK(RemoveUsingNamespace)
@@ -83,11 +86,6 @@
"", Ctx.getLangOpts());
}
-// Returns true iff the parent of the Node is a TUDecl.
-bool isTopLevelDecl(const SelectionTree::Node *Node) {
- return Node->Parent && Node->Parent->ASTNode.get<TranslationUnitDecl>();
-}
-
// Returns the first visible context that contains this DeclContext.
// For example: Returns ns1 for S1 and a.
// namespace ns1 {
@@ -108,7 +106,8 @@
TargetDirective = CA->ASTNode.get<UsingDirectiveDecl>();
if (!TargetDirective)
return false;
- if (!dyn_cast<Decl>(TargetDirective->getDeclContext()))
+ auto DeclCtx = TargetDirective->getDeclContext();
+ if (!dyn_cast<Decl>(DeclCtx))
return false;
// FIXME: Unavailable for namespaces containing using-namespace decl.
// It is non-trivial to deal with cases where identifiers come from the inner
@@ -122,7 +121,9 @@
// We need to make this aware of the transitive using-namespace decls.
if (!TargetDirective->getNominatedNamespace()->using_directives().empty())
return false;
- return isTopLevelDecl(CA);
+ // The containing context must be a namespace or a TU decl.
+ ContainingNS = dyn_cast<NamespaceDecl>(DeclCtx);
+ return ContainingNS || DeclCtx->isTranslationUnit();;
}
Expected<Tweak::Effect> RemoveUsingNamespace::apply(const Selection &Inputs) {
@@ -141,43 +142,45 @@
}
// Collect all references to symbols from the namespace for which we're
- // removing the directive.
- std::vector<SourceLocation> IdentsToQualify;
- for (auto &D : Inputs.AST.getLocalTopLevelDecls()) {
- findExplicitReferences(D, [&](ReferenceLoc Ref) {
- if (Ref.Qualifier)
- return; // This reference is already qualified.
-
- for (auto *T : Ref.Targets) {
- if (!visibleContext(T->getDeclContext())
- ->Equals(TargetDirective->getNominatedNamespace()))
- return;
- }
- SourceLocation Loc = Ref.NameLoc;
- if (Loc.isMacroID()) {
- // Avoid adding qualifiers before macro expansions, it's probably
- // incorrect, e.g.
- // namespace std { int foo(); }
- // #define FOO 1 + foo()
- // using namespace foo; // provides matrix
- // auto x = FOO; // Must not changed to auto x = std::FOO
- if (!SM.isMacroArgExpansion(Loc))
- return; // FIXME: report a warning to the users.
- Loc = SM.getFileLoc(Ref.NameLoc);
- }
- assert(Loc.isFileID());
- if (SM.getFileID(Loc) != SM.getMainFileID())
- return; // FIXME: report these to the user as warnings?
- if (SM.isBeforeInTranslationUnit(Loc, FirstUsingDirectiveLoc))
- return; // Directive was not visible before this point.
- IdentsToQualify.push_back(Loc);
- });
- }
- // Remove duplicates.
- llvm::sort(IdentsToQualify);
- IdentsToQualify.erase(
- std::unique(IdentsToQualify.begin(), IdentsToQualify.end()),
- IdentsToQualify.end());
+ // removing the directive. Replace their current qualifier with the TargetNS.
+ std::vector<CharSourceRange> QualifierReplacements;
+ auto SelectRefToQualify = [&](ReferenceLoc Ref) {
+ if (Ref.Targets.empty())
+ return;
+ for (auto *T : Ref.Targets) {
+ if (!visibleContext(T->getDeclContext())
+ ->Equals(TargetDirective->getNominatedNamespace()))
+ return;
+ }
+ SourceLocation Loc = Ref.NameLoc;
+ if (Loc.isMacroID()) {
+ // Avoid adding qualifiers before macro expansions, it's probably
+ // incorrect, e.g.
+ // namespace std { int foo(); }
+ // #define FOO 1 + foo()
+ // using namespace foo; // provides matrix
+ // auto x = FOO; // Must not changed to auto x = std::FOO
+ if (!SM.isMacroArgExpansion(Loc))
+ return; // FIXME: report a warning to the users.
+ Loc = SM.getFileLoc(Ref.NameLoc);
+ }
+ assert(Loc.isFileID());
+ if (SM.getFileID(Loc) != SM.getMainFileID())
+ return; // FIXME: report these to the user as warnings?
+ if (SM.isBeforeInTranslationUnit(Loc, FirstUsingDirectiveLoc))
+ return; // Directive was not visible before this point.
+
+ CharSourceRange RemoveQualifier;
+ RemoveQualifier.setBegin(Ref.Qualifier ? Ref.Qualifier.getBeginLoc() : Loc);
+ RemoveQualifier.setEnd(Loc);
+ QualifierReplacements.push_back(RemoveQualifier);
+ };
+
+ if (ContainingNS)
+ findExplicitReferences(ContainingNS, SelectRefToQualify);
+ else
+ for (auto &D : Inputs.AST.getLocalTopLevelDecls())
+ findExplicitReferences(D, SelectRefToQualify);
// Produce replacements to remove the using directives.
tooling::Replacements R;
@@ -190,9 +193,9 @@
}
// Produce replacements to add the qualifiers.
std::string Qualifier = printUsingNamespaceName(Ctx, *TargetDirective) + "::";
- for (auto Loc : IdentsToQualify) {
- if (auto Err = R.add(tooling::Replacement(Ctx.getSourceManager(), Loc,
- /*Length=*/0, Qualifier)))
+ for (auto RemoveRange : QualifierReplacements) {
+ if (auto Err = R.add(tooling::Replacement(Ctx.getSourceManager(),
+ RemoveRange, Qualifier)))
return std::move(Err);
}
return Effect::mainFileEdit(SM, std::move(R));
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
