[PATCH] D53489: [change-namespace] Enhance detection of conflicting namespaces.

[Eric Liu via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL344897: [change-namespace] Enhance detection of conflicting 
namespaces. (authored by ioeric, committed by ).
Herald added a subscriber: llvm-commits.

Repository:
  rL LLVM

https://reviews.llvm.org/D53489

Files:
  clang-tools-extra/trunk/change-namespace/ChangeNamespace.cpp
  clang-tools-extra/trunk/unittests/change-namespace/ChangeNamespaceTests.cpp

Index: clang-tools-extra/trunk/unittests/change-namespace/ChangeNamespaceTests.cpp
===
--- clang-tools-extra/trunk/unittests/change-namespace/ChangeNamespaceTests.cpp
+++ clang-tools-extra/trunk/unittests/change-namespace/ChangeNamespaceTests.cpp
@@ -2244,6 +2244,39 @@
   EXPECT_EQ(format(Expected), runChangeNamespaceOnCode(Code));
 }
 
+TEST_F(ChangeNamespaceTest, FullyQualifyConflictNamespace) {
+  std::string Code =
+  "namespace x { namespace util { class Some {}; } }\n"
+  "namespace x { namespace y {namespace base { class Base {}; } } }\n"
+  "namespace util { class Status {}; }\n"
+  "namespace base { class Base {}; }\n"
+  "namespace na {\n"
+  "namespace nb {\n"
+  "void f() {\n"
+  "  util::Status s1; x::util::Some s2;\n"
+  "  base::Base b1; x::y::base::Base b2;\n"
+  "}\n"
+  "} // namespace nb\n"
+  "} // namespace na\n";
+
+  std::string Expected =
+  "namespace x { namespace util { class Some {}; } }\n"
+  "namespace x { namespace y {namespace base { class Base {}; } } }\n"
+  "namespace util { class Status {}; }\n"
+  "namespace base { class Base {}; }\n"
+  "\n"
+  "namespace x {\n"
+  "namespace y {\n"
+  "void f() {\n"
+  "  ::util::Status s1; util::Some s2;\n"
+  "  ::base::Base b1; base::Base b2;\n"
+  "}\n"
+  "} // namespace y\n"
+  "} // namespace x\n";
+
+  EXPECT_EQ(format(Expected), runChangeNamespaceOnCode(Code));
+}
+
 } // anonymous namespace
 } // namespace change_namespace
 } // namespace clang
Index: clang-tools-extra/trunk/change-namespace/ChangeNamespace.cpp
===
--- clang-tools-extra/trunk/change-namespace/ChangeNamespace.cpp
+++ clang-tools-extra/trunk/change-namespace/ChangeNamespace.cpp
@@ -7,8 +7,10 @@
 //
 //===--===//
 #include "ChangeNamespace.h"
+#include "clang/AST/ASTContext.h"
 #include "clang/Format/Format.h"
 #include "clang/Lex/Lexer.h"
+#include "llvm/Support/Casting.h"
 #include "llvm/Support/ErrorHandling.h"
 
 using namespace clang::ast_matchers;
@@ -283,23 +285,48 @@
 }
 
 // Given a qualified symbol name, returns true if the symbol will be
-// incorrectly qualified without leading "::".
-bool conflictInNamespace(llvm::StringRef QualifiedSymbol,
+// incorrectly qualified without leading "::". For example, a symbol
+// "nx::ny::Foo" in namespace "na::nx::ny" without leading "::"; a symbol
+// "util::X" in namespace "na" can potentially conflict with "na::util" (if this
+// exists).
+bool conflictInNamespace(const ASTContext , llvm::StringRef QualifiedSymbol,
  llvm::StringRef Namespace) {
   auto SymbolSplitted = splitSymbolName(QualifiedSymbol.trim(":"));
   assert(!SymbolSplitted.empty());
   SymbolSplitted.pop_back();  // We are only interested in namespaces.
 
-  if (SymbolSplitted.size() > 1 && !Namespace.empty()) {
+  if (SymbolSplitted.size() >= 1 && !Namespace.empty()) {
+auto SymbolTopNs = SymbolSplitted.front();
 auto NsSplitted = splitSymbolName(Namespace.trim(":"));
 assert(!NsSplitted.empty());
-// We do not check the outermost namespace since it would not be a conflict
-// if it equals to the symbol's outermost namespace and the symbol name
-// would have been shortened.
+
+auto LookupDecl = [](const Decl ,
+ llvm::StringRef Name) -> const NamedDecl * {
+  const auto *DC = llvm::dyn_cast();
+  if (!DC)
+return nullptr;
+  auto LookupRes = DC->lookup(DeclarationName((Name)));
+  if (LookupRes.empty())
+return nullptr;
+  return LookupRes.front();
+};
+// We do not check the outermost namespace since it would not be a
+// conflict if it equals to the symbol's outermost namespace and the
+// symbol name would have been shortened.
+const NamedDecl *Scope =
+LookupDecl(*AST.getTranslationUnitDecl(), NsSplitted.front());
 for (auto I = NsSplitted.begin() + 1, E = NsSplitted.end(); I != E; ++I) {
-  if (*I == SymbolSplitted.front())
+  if (*I == SymbolTopNs) // Handles "::ny" in "::nx::ny" case.
 return true;
+  // Handles "::util" and "::nx::util" conflicts.
+  if (Scope) {
+if (LookupDecl(*Scope, SymbolTopNs))
+  return true;
+Scope = LookupDecl(*Scope, *I);
+  }
 }
+if (Scope && LookupDecl(*Scope, 

[PATCH] D53489: [change-namespace] Enhance detection of conflicting namespaces.

[Haojian Wu via Phabricator via cfe-commits]

hokein accepted this revision.
hokein added a comment.
This revision is now accepted and ready to land.

LGTM


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D53489



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53489: [change-namespace] Enhance detection of conflicting namespaces.

[Eric Liu via Phabricator via cfe-commits]

ioeric created this revision.
ioeric added a reviewer: hokein.
Herald added a subscriber: cfe-commits.

For example:

  namespace util { class Base; }
  
  namespace new {
  namespace util { class Internal; }
  }
  
  namespace old {
  util::Base b1;
  }

When changing `old::` to `new::`, `util::` in namespace "new::" will conflict
with "new::util::" unless a leading "::" is added.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D53489

Files:
  change-namespace/ChangeNamespace.cpp
  unittests/change-namespace/ChangeNamespaceTests.cpp

Index: unittests/change-namespace/ChangeNamespaceTests.cpp
===
--- unittests/change-namespace/ChangeNamespaceTests.cpp
+++ unittests/change-namespace/ChangeNamespaceTests.cpp
@@ -2244,6 +2244,39 @@
   EXPECT_EQ(format(Expected), runChangeNamespaceOnCode(Code));
 }
 
+TEST_F(ChangeNamespaceTest, FullyQualifyConflictNamespace) {
+  std::string Code =
+  "namespace x { namespace util { class Some {}; } }\n"
+  "namespace x { namespace y {namespace base { class Base {}; } } }\n"
+  "namespace util { class Status {}; }\n"
+  "namespace base { class Base {}; }\n"
+  "namespace na {\n"
+  "namespace nb {\n"
+  "void f() {\n"
+  "  util::Status s1; x::util::Some s2;\n"
+  "  base::Base b1; x::y::base::Base b2;\n"
+  "}\n"
+  "} // namespace nb\n"
+  "} // namespace na\n";
+
+  std::string Expected =
+  "namespace x { namespace util { class Some {}; } }\n"
+  "namespace x { namespace y {namespace base { class Base {}; } } }\n"
+  "namespace util { class Status {}; }\n"
+  "namespace base { class Base {}; }\n"
+  "\n"
+  "namespace x {\n"
+  "namespace y {\n"
+  "void f() {\n"
+  "  ::util::Status s1; util::Some s2;\n"
+  "  ::base::Base b1; base::Base b2;\n"
+  "}\n"
+  "} // namespace y\n"
+  "} // namespace x\n";
+
+  EXPECT_EQ(format(Expected), runChangeNamespaceOnCode(Code));
+}
+
 } // anonymous namespace
 } // namespace change_namespace
 } // namespace clang
Index: change-namespace/ChangeNamespace.cpp
===
--- change-namespace/ChangeNamespace.cpp
+++ change-namespace/ChangeNamespace.cpp
@@ -7,8 +7,10 @@
 //
 //===--===//
 #include "ChangeNamespace.h"
+#include "clang/AST/ASTContext.h"
 #include "clang/Format/Format.h"
 #include "clang/Lex/Lexer.h"
+#include "llvm/Support/Casting.h"
 #include "llvm/Support/ErrorHandling.h"
 
 using namespace clang::ast_matchers;
@@ -283,23 +285,48 @@
 }
 
 // Given a qualified symbol name, returns true if the symbol will be
-// incorrectly qualified without leading "::".
-bool conflictInNamespace(llvm::StringRef QualifiedSymbol,
+// incorrectly qualified without leading "::". For example, a symbol
+// "nx::ny::Foo" in namespace "na::nx::ny" without leading "::"; a symbol
+// "util::X" in namespace "na" can potentially conflict with "na::util" (if this
+// exists).
+bool conflictInNamespace(const ASTContext , llvm::StringRef QualifiedSymbol,
  llvm::StringRef Namespace) {
   auto SymbolSplitted = splitSymbolName(QualifiedSymbol.trim(":"));
   assert(!SymbolSplitted.empty());
   SymbolSplitted.pop_back();  // We are only interested in namespaces.
 
-  if (SymbolSplitted.size() > 1 && !Namespace.empty()) {
+  if (SymbolSplitted.size() >= 1 && !Namespace.empty()) {
+auto SymbolTopNs = SymbolSplitted.front();
 auto NsSplitted = splitSymbolName(Namespace.trim(":"));
 assert(!NsSplitted.empty());
-// We do not check the outermost namespace since it would not be a conflict
-// if it equals to the symbol's outermost namespace and the symbol name
-// would have been shortened.
+
+auto LookupDecl = [](const Decl ,
+ llvm::StringRef Name) -> const NamedDecl * {
+  const auto *DC = llvm::dyn_cast();
+  if (!DC)
+return nullptr;
+  auto LookupRes = DC->lookup(DeclarationName((Name)));
+  if (LookupRes.empty())
+return nullptr;
+  return LookupRes.front();
+};
+// We do not check the outermost namespace since it would not be a
+// conflict if it equals to the symbol's outermost namespace and the
+// symbol name would have been shortened.
+const NamedDecl *Scope =
+LookupDecl(*AST.getTranslationUnitDecl(), NsSplitted.front());
 for (auto I = NsSplitted.begin() + 1, E = NsSplitted.end(); I != E; ++I) {
-  if (*I == SymbolSplitted.front())
+  if (*I == SymbolTopNs) // Handles "::ny" in "::nx::ny" case.
 return true;
+  // Handles "::util" and "::nx::util" conflicts.
+  if (Scope) {
+if (LookupDecl(*Scope, SymbolTopNs))
+  return true;
+Scope = LookupDecl(*Scope, *I);
+  }
 }
+if (Scope && LookupDecl(*Scope, SymbolTopNs))
+  return true;
   }