Re: [PATCH] D24183: A clang tool for changing surrouding namespaces of class/function definitions.
This revision was automatically updated to reflect the committed changes.
Closed by commit rL281918: A clang tool for changing surrouding namespaces of
class/function definitions. (authored by ioeric).
Changed prior to commit:
https://reviews.llvm.org/D24183?vs=71689&id=71848#toc
Repository:
rL LLVM
https://reviews.llvm.org/D24183
Files:
clang-tools-extra/trunk/CMakeLists.txt
clang-tools-extra/trunk/change-namespace/CMakeLists.txt
clang-tools-extra/trunk/change-namespace/ChangeNamespace.cpp
clang-tools-extra/trunk/change-namespace/ChangeNamespace.h
clang-tools-extra/trunk/change-namespace/tool/CMakeLists.txt
clang-tools-extra/trunk/change-namespace/tool/ClangChangeNamespace.cpp
clang-tools-extra/trunk/test/CMakeLists.txt
clang-tools-extra/trunk/test/change-namespace/simple-move.cpp
clang-tools-extra/trunk/unittests/CMakeLists.txt
clang-tools-extra/trunk/unittests/change-namespace/CMakeLists.txt
clang-tools-extra/trunk/unittests/change-namespace/ChangeNamespaceTests.cpp
Index: clang-tools-extra/trunk/unittests/change-namespace/CMakeLists.txt
===
--- clang-tools-extra/trunk/unittests/change-namespace/CMakeLists.txt
+++ clang-tools-extra/trunk/unittests/change-namespace/CMakeLists.txt
@@ -0,0 +1,28 @@
+set(LLVM_LINK_COMPONENTS
+ support
+ )
+
+get_filename_component(CHANGE_NAMESPACE_SOURCE_DIR
+ ${CMAKE_CURRENT_SOURCE_DIR}/../../change-namespace REALPATH)
+include_directories(
+ ${CHANGE_NAMESPACE_SOURCE_DIR}
+ )
+
+# We'd like clang/unittests/Tooling/RewriterTestContext.h in the test.
+include_directories(${CLANG_SOURCE_DIR})
+
+add_extra_unittest(ChangeNamespaceTests
+ ChangeNamespaceTests.cpp
+ )
+
+target_link_libraries(ChangeNamespaceTests
+ clangAST
+ clangASTMatchers
+ clangBasic
+ clangChangeNamespace
+ clangFormat
+ clangFrontend
+ clangRewrite
+ clangTooling
+ clangToolingCore
+ )
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
@@ -0,0 +1,234 @@
+//===-- ChangeNamespaceTests.cpp - Change namespace unit tests ---*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "ChangeNamespace.h"
+#include "unittests/Tooling/RewriterTestContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Basic/FileManager.h"
+#include "clang/Basic/FileSystemOptions.h"
+#include "clang/Basic/VirtualFileSystem.h"
+#include "clang/Format/Format.h"
+#include "clang/Frontend/CompilerInstance.h"
+#include "clang/Frontend/PCHContainerOperations.h"
+#include "clang/Tooling/Refactoring.h"
+#include "clang/Tooling/Tooling.h"
+#include "llvm/ADT/IntrusiveRefCntPtr.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/MemoryBuffer.h"
+#include "gtest/gtest.h"
+#include
+#include
+#include
+
+namespace clang {
+namespace change_namespace {
+namespace {
+
+class ChangeNamespaceTest : public ::testing::Test {
+public:
+ std::string runChangeNamespaceOnCode(llvm::StringRef Code) {
+clang::RewriterTestContext Context;
+clang::FileID ID = Context.createInMemoryFile(FileName, Code);
+
+std::map FileToReplacements;
+change_namespace::ChangeNamespaceTool NamespaceTool(
+OldNamespace, NewNamespace, FilePattern, &FileToReplacements);
+ast_matchers::MatchFinder Finder;
+NamespaceTool.registerMatchers(&Finder);
+std::unique_ptr Factory =
+tooling::newFrontendActionFactory(&Finder);
+tooling::runToolOnCodeWithArgs(Factory->create(), Code, {"-std=c++11"},
+ FileName);
+formatAndApplyAllReplacements(FileToReplacements, Context.Rewrite);
+return format(Context.getRewrittenText(ID));
+ }
+
+ std::string format(llvm::StringRef Code) {
+tooling::Replacements Replaces = format::reformat(
+format::getLLVMStyle(), Code, {tooling::Range(0, Code.size())});
+auto ChangedCode = tooling::applyAllReplacements(Code, Replaces);
+EXPECT_TRUE(static_cast(ChangedCode));
+if (!ChangedCode) {
+ llvm::errs() << llvm::toString(ChangedCode.takeError());
+ return "";
+}
+return *ChangedCode;
+ }
+
+protected:
+ std::string FileName = "input.cc";
+ std::string OldNamespace = "na::nb";
+ std::string NewNamespace = "x::y";
+ std::string FilePattern = "input.cc";
+};
+
+TEST_F(ChangeNamespaceTest, NoMatchingNamespace) {
+ std::string Code = "namespace na {\n"
+ "namespace nx {\n"
+ "class A {};\n"
+ "} // namespace nx\n"
+ "} // n
Re: [PATCH] D24183: A clang tool for changing surrouding namespaces of class/function definitions.
ioeric updated this revision to Diff 71689.
ioeric marked 4 inline comments as done.
ioeric added a comment.
- Updated commenting, and fix a bug in the binary.
https://reviews.llvm.org/D24183
Files:
CMakeLists.txt
change-namespace/CMakeLists.txt
change-namespace/ChangeNamespace.cpp
change-namespace/ChangeNamespace.h
change-namespace/tool/CMakeLists.txt
change-namespace/tool/ClangChangeNamespace.cpp
test/CMakeLists.txt
test/change-namespace/simple-move.cpp
unittests/CMakeLists.txt
unittests/change-namespace/CMakeLists.txt
unittests/change-namespace/ChangeNamespaceTests.cpp
Index: unittests/change-namespace/ChangeNamespaceTests.cpp
===
--- /dev/null
+++ unittests/change-namespace/ChangeNamespaceTests.cpp
@@ -0,0 +1,234 @@
+//===-- ChangeNamespaceTests.cpp - Change namespace unit tests ---*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "ChangeNamespace.h"
+#include "unittests/Tooling/RewriterTestContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Basic/FileManager.h"
+#include "clang/Basic/FileSystemOptions.h"
+#include "clang/Basic/VirtualFileSystem.h"
+#include "clang/Format/Format.h"
+#include "clang/Frontend/CompilerInstance.h"
+#include "clang/Frontend/PCHContainerOperations.h"
+#include "clang/Tooling/Refactoring.h"
+#include "clang/Tooling/Tooling.h"
+#include "llvm/ADT/IntrusiveRefCntPtr.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/MemoryBuffer.h"
+#include "gtest/gtest.h"
+#include
+#include
+#include
+
+namespace clang {
+namespace change_namespace {
+namespace {
+
+class ChangeNamespaceTest : public ::testing::Test {
+public:
+ std::string runChangeNamespaceOnCode(llvm::StringRef Code) {
+clang::RewriterTestContext Context;
+clang::FileID ID = Context.createInMemoryFile(FileName, Code);
+
+std::map FileToReplacements;
+change_namespace::ChangeNamespaceTool NamespaceTool(
+OldNamespace, NewNamespace, FilePattern, &FileToReplacements);
+ast_matchers::MatchFinder Finder;
+NamespaceTool.registerMatchers(&Finder);
+std::unique_ptr Factory =
+tooling::newFrontendActionFactory(&Finder);
+tooling::runToolOnCodeWithArgs(Factory->create(), Code, {"-std=c++11"},
+ FileName);
+formatAndApplyAllReplacements(FileToReplacements, Context.Rewrite);
+return format(Context.getRewrittenText(ID));
+ }
+
+ std::string format(llvm::StringRef Code) {
+tooling::Replacements Replaces = format::reformat(
+format::getLLVMStyle(), Code, {tooling::Range(0, Code.size())});
+auto ChangedCode = tooling::applyAllReplacements(Code, Replaces);
+EXPECT_TRUE(static_cast(ChangedCode));
+if (!ChangedCode) {
+ llvm::errs() << llvm::toString(ChangedCode.takeError());
+ return "";
+}
+return *ChangedCode;
+ }
+
+protected:
+ std::string FileName = "input.cc";
+ std::string OldNamespace = "na::nb";
+ std::string NewNamespace = "x::y";
+ std::string FilePattern = "input.cc";
+};
+
+TEST_F(ChangeNamespaceTest, NoMatchingNamespace) {
+ std::string Code = "namespace na {\n"
+ "namespace nx {\n"
+ "class A {};\n"
+ "} // namespace nx\n"
+ "} // namespace na\n";
+ std::string Expected = "namespace na {\n"
+ "namespace nx {\n"
+ "class A {};\n"
+ "} // namespace nx\n"
+ "} // namespace na\n";
+ EXPECT_EQ(format(Expected), runChangeNamespaceOnCode(Code));
+}
+
+TEST_F(ChangeNamespaceTest, SimpleMoveWithoutTypeRefs) {
+ std::string Code = "namespace na {\n"
+ "namespace nb {\n"
+ "class A {};\n"
+ "} // namespace nb\n"
+ "} // namespace na\n";
+ std::string Expected = "\n\n"
+ "namespace x {\n"
+ "namespace y {\n"
+ "class A {};\n"
+ "} // namespace y\n"
+ "} // namespace x\n";
+ EXPECT_EQ(format(Expected), runChangeNamespaceOnCode(Code));
+}
+
+TEST_F(ChangeNamespaceTest, SimpleMoveIntoAnotherNestedNamespace) {
+ NewNamespace = "na::nc";
+ std::string Code = "namespace na {\n"
+ "namespace nb {\n"
+ "class A {};\n"
+ "} // namespace nb\n"
+ "} // namespace na\n";
+ std::string Expected = "namespace na {\n"
+ "\n"
+ "namespace nc {\n"
+ "class A {};\n"
+ "} // namespace nc\n"
+
Re: [PATCH] D24183: A clang tool for changing surrouding namespaces of class/function definitions.
omtcyfz accepted this revision. omtcyfz added a comment. I have no other objections aswell. LGTM. https://reviews.llvm.org/D24183 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D24183: A clang tool for changing surrouding namespaces of class/function definitions.
hokein accepted this revision.
hokein added a comment.
This revision is now accepted and ready to land.
The patch looks good to me now.
Comment at: change-namespace/ChangeNamespace.h:44
@@ +43,3 @@
+class ChangeNamespaceTool : ast_matchers::MatchFinder::MatchCallback {
+public:
+ // Moves code in the old namespace `OldNs` to the new namespace `NewNs` in
You forgot this one.
Comment at: change-namespace/ChangeNamespace.h:101
@@ +100,3 @@
+ std::string FallbackStyle;
+ std::map &FileToReplacements;
+ // A fully qualified name of the old namespace without "::" prefix, e.g.
Would be clearer to add comment describing the kinds of these replacements
(e.g. deleting the forward declarations, replacing the old qualifiers with the
new shortest qualified name).
https://reviews.llvm.org/D24183
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D24183: A clang tool for changing surrouding namespaces of class/function definitions.
omtcyfz added inline comments. Comment at: change-namespace/ChangeNamespace.cpp:448 @@ +447,3 @@ + continue; +const std::string &FilePath = FileAndNsMoves.first; +auto &Replaces = FileToReplacements[FilePath]; ioeric wrote: > omtcyfz wrote: > > `StringRef` here too. > If this was a `StringRef`, then each map access would require an implicit > conversion from `StringRef` to `std::string`, which is expensive. And this is > already a reference anyway. Ah, I see. Yes, that's true. Sorry. https://reviews.llvm.org/D24183 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D24183: A clang tool for changing surrouding namespaces of class/function definitions.
ioeric marked an inline comment as done. Comment at: change-namespace/ChangeNamespace.cpp:448 @@ +447,3 @@ + continue; +const std::string &FilePath = FileAndNsMoves.first; +auto &Replaces = FileToReplacements[FilePath]; omtcyfz wrote: > `StringRef` here too. If this was a `StringRef`, then each map access would require an implicit conversion from `StringRef` to `std::string`, which is expensive. And this is already a reference anyway. https://reviews.llvm.org/D24183 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D24183: A clang tool for changing surrouding namespaces of class/function definitions.
omtcyfz added inline comments. Comment at: change-namespace/ChangeNamespace.cpp:448 @@ +447,3 @@ + continue; +const std::string &FilePath = FileAndNsMoves.first; +auto &Replaces = FileToReplacements[FilePath]; `StringRef` here too. https://reviews.llvm.org/D24183 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D24183: A clang tool for changing surrouding namespaces of class/function definitions.
ioeric added inline comments.
Comment at: change-namespace/ChangeNamespace.cpp:200
@@ +199,3 @@
+ while (!NsSplitted.empty()) {
+// FIXME: consider code style for comments.
+Code = ("namespace " + NsSplitted.back() + " {\n" + Code +
hokein wrote:
> Doesn't `formatAndApplyAllReplacements` format the comments for us?
formatter only adds/removes white spaces I think.
Comment at: change-namespace/ChangeNamespace.cpp:400
@@ +399,3 @@
+ assert(Consumed && "Expect OldNS to start with OldNamespace.");
+ (void)Consumed;
+ const std::string NewNs = (NewNamespace + Postfix).str();
hokein wrote:
> how about `assert(Postfix.consume_front(OldNamespace) && "Expect OldNS to
> start with OldNamespace.");`?
The problem with this is that `Postfix.consume_front(OldNamespace)` won't be
executed if assertion is turned off.
https://reviews.llvm.org/D24183
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D24183: A clang tool for changing surrouding namespaces of class/function definitions.
ioeric updated this revision to Diff 71605.
ioeric marked 8 inline comments as done.
ioeric added a comment.
- Addressed review comments.
https://reviews.llvm.org/D24183
Files:
CMakeLists.txt
change-namespace/CMakeLists.txt
change-namespace/ChangeNamespace.cpp
change-namespace/ChangeNamespace.h
change-namespace/tool/CMakeLists.txt
change-namespace/tool/ClangChangeNamespace.cpp
test/CMakeLists.txt
test/change-namespace/simple-move.cpp
unittests/CMakeLists.txt
unittests/change-namespace/CMakeLists.txt
unittests/change-namespace/ChangeNamespaceTests.cpp
Index: unittests/change-namespace/ChangeNamespaceTests.cpp
===
--- /dev/null
+++ unittests/change-namespace/ChangeNamespaceTests.cpp
@@ -0,0 +1,234 @@
+//===-- ChangeNamespaceTests.cpp - Change namespace unit tests ---*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "ChangeNamespace.h"
+#include "unittests/Tooling/RewriterTestContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Basic/FileManager.h"
+#include "clang/Basic/FileSystemOptions.h"
+#include "clang/Basic/VirtualFileSystem.h"
+#include "clang/Format/Format.h"
+#include "clang/Frontend/CompilerInstance.h"
+#include "clang/Frontend/PCHContainerOperations.h"
+#include "clang/Tooling/Refactoring.h"
+#include "clang/Tooling/Tooling.h"
+#include "llvm/ADT/IntrusiveRefCntPtr.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/MemoryBuffer.h"
+#include "gtest/gtest.h"
+#include
+#include
+#include
+
+namespace clang {
+namespace change_namespace {
+namespace {
+
+class ChangeNamespaceTest : public ::testing::Test {
+public:
+ std::string runChangeNamespaceOnCode(llvm::StringRef Code) {
+clang::RewriterTestContext Context;
+clang::FileID ID = Context.createInMemoryFile(FileName, Code);
+
+std::map FileToReplacements;
+change_namespace::ChangeNamespaceTool NamespaceTool(
+OldNamespace, NewNamespace, FilePattern, &FileToReplacements);
+ast_matchers::MatchFinder Finder;
+NamespaceTool.registerMatchers(&Finder);
+std::unique_ptr Factory =
+tooling::newFrontendActionFactory(&Finder);
+tooling::runToolOnCodeWithArgs(Factory->create(), Code, {"-std=c++11"},
+ FileName);
+formatAndApplyAllReplacements(FileToReplacements, Context.Rewrite);
+return format(Context.getRewrittenText(ID));
+ }
+
+ std::string format(llvm::StringRef Code) {
+tooling::Replacements Replaces = format::reformat(
+format::getLLVMStyle(), Code, {tooling::Range(0, Code.size())});
+auto ChangedCode = tooling::applyAllReplacements(Code, Replaces);
+EXPECT_TRUE(static_cast(ChangedCode));
+if (!ChangedCode) {
+ llvm::errs() << llvm::toString(ChangedCode.takeError());
+ return "";
+}
+return *ChangedCode;
+ }
+
+protected:
+ std::string FileName = "input.cc";
+ std::string OldNamespace = "na::nb";
+ std::string NewNamespace = "x::y";
+ std::string FilePattern = "input.cc";
+};
+
+TEST_F(ChangeNamespaceTest, NoMatchingNamespace) {
+ std::string Code = "namespace na {\n"
+ "namespace nx {\n"
+ "class A {};\n"
+ "} // namespace nx\n"
+ "} // namespace na\n";
+ std::string Expected = "namespace na {\n"
+ "namespace nx {\n"
+ "class A {};\n"
+ "} // namespace nx\n"
+ "} // namespace na\n";
+ EXPECT_EQ(format(Expected), runChangeNamespaceOnCode(Code));
+}
+
+TEST_F(ChangeNamespaceTest, SimpleMoveWithoutTypeRefs) {
+ std::string Code = "namespace na {\n"
+ "namespace nb {\n"
+ "class A {};\n"
+ "} // namespace nb\n"
+ "} // namespace na\n";
+ std::string Expected = "\n\n"
+ "namespace x {\n"
+ "namespace y {\n"
+ "class A {};\n"
+ "} // namespace y\n"
+ "} // namespace x\n";
+ EXPECT_EQ(format(Expected), runChangeNamespaceOnCode(Code));
+}
+
+TEST_F(ChangeNamespaceTest, SimpleMoveIntoAnotherNestedNamespace) {
+ NewNamespace = "na::nc";
+ std::string Code = "namespace na {\n"
+ "namespace nb {\n"
+ "class A {};\n"
+ "} // namespace nb\n"
+ "} // namespace na\n";
+ std::string Expected = "namespace na {\n"
+ "\n"
+ "namespace nc {\n"
+ "class A {};\n"
+ "} // namespace nc\n"
+ "} // names
Re: [PATCH] D24183: A clang tool for changing surrouding namespaces of class/function definitions.
hokein added inline comments.
Comment at: change-namespace/ChangeNamespace.cpp:200
@@ +199,3 @@
+ while (!NsSplitted.empty()) {
+// FIXME: consider code style for comments.
+Code = ("namespace " + NsSplitted.back() + " {\n" + Code +
Doesn't `formatAndApplyAllReplacements` format the comments for us?
Comment at: change-namespace/ChangeNamespace.cpp:213
@@ +212,3 @@
+llvm::StringRef OldNs, llvm::StringRef NewNs,
+const std::string &FilePattern,
+std::map *FileToReplacements,
StringRef.
Comment at: change-namespace/ChangeNamespace.cpp:242
@@ +241,3 @@
+ // Match old namespace blocks.
+ Finder->addMatcher(namespaceDecl(hasName(OldNamespace),
+ isExpansionInFileMatching(FilePattern))
Actually, `hasName` matches the name which ends with `OldNamespace`, which
doesn't align with the your comments in `OldNamespace` member variable.
Comment at: change-namespace/ChangeNamespace.cpp:373
@@ +372,3 @@
+ // code from old namespace to new namespace.
+ // Insertion information is stores in `InsertFwdDecls` and actual
+ // insertion will be performed in `onEndOfTranslationUnit`.
s/stores/stored
Comment at: change-namespace/ChangeNamespace.cpp:400
@@ +399,3 @@
+ assert(Consumed && "Expect OldNS to start with OldNamespace.");
+ (void)Consumed;
+ const std::string NewNs = (NewNamespace + Postfix).str();
how about `assert(Postfix.consume_front(OldNamespace) && "Expect OldNS to start
with OldNamespace.");`?
Comment at: change-namespace/ChangeNamespace.cpp:421
@@ +420,3 @@
+
+void ChangeNamespaceTool::fixTypeLoc(
+const ast_matchers::MatchFinder::MatchResult &Result, SourceLocation Start,
Could you add some comments describe what stuff does this function do? The name
`fixTypeLoc` doesn't explain too much. Looks like this function is replacing
the qualifiers in TypeLoc.
Comment at: change-namespace/ChangeNamespace.h:43
@@ +42,3 @@
+// FIXME: support moving typedef, enums across namespaces.
+class ChangeNamespaceTool : ast_matchers::MatchFinder::MatchCallback {
+public:
I think you are missing `public` keyword here, otherwise, it will be private
inheritance.
Comment at: change-namespace/tool/ClangChangeNamespace.cpp:46
@@ +45,3 @@
+
+cl::OptionCategory ChangeNamespaceCategory("Tool options");
+
change to "Change namespace"?
Comment at: change-namespace/tool/ClangChangeNamespace.cpp:106
@@ +105,3 @@
+outs() << "== " << File << " ==\n";
+Rewrite.getEditBuffer(ID).write(llvm::outs());
+outs() << "\n\n";
Might be add a FIXME?
https://reviews.llvm.org/D24183
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D24183: A clang tool for changing surrouding namespaces of class/function definitions.
ioeric added a comment. PIng https://reviews.llvm.org/D24183 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D24183: A clang tool for changing surrouding namespaces of class/function definitions.
ioeric added inline comments. Comment at: change-namespace/ChangeNamespace.cpp:359 @@ +358,3 @@ + End, tok::semi, *Result.SourceManager, Result.Context->getLangOpts(), + /*SkipTrailingWhitespaceAndNewLine=*/true); + if (AfterSemi.isValid()) omtcyfz wrote: > The indentation seems a bit off. Semicolon is on the next line for some > reason and I think `/*SkipTrailingWhitespaceAndNewLine=*/` should be `/* > SkipTrailingWhitespaceAndNewLine */`. > > UPD: Semicolon came back :) It seems that Phabricator is not behaving well recently. I saw similar indentation inconsistency in other patches as well. For the comment, I think either way is fine. You can find both styles in the code base, and I'm not sure which is the standard. Samples: https://github.com/llvm-mirror/clang/blob/master/include/clang/Basic/SourceManager.h#L782 https://github.com/llvm-mirror/clang/blob/master/include/clang/Basic/SourceManager.h#L847 https://reviews.llvm.org/D24183 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D24183: A clang tool for changing surrouding namespaces of class/function definitions.
omtcyfz added inline comments. Comment at: change-namespace/ChangeNamespace.cpp:359 @@ +358,3 @@ + End, tok::semi, *Result.SourceManager, Result.Context->getLangOpts(), + /*SkipTrailingWhitespaceAndNewLine=*/true); + if (AfterSemi.isValid()) The indentation seems a bit off. Semicolon is on the next line for some reason and I think `/*SkipTrailingWhitespaceAndNewLine=*/` should be `/* SkipTrailingWhitespaceAndNewLine */`. UPD: Semicolon came back :) https://reviews.llvm.org/D24183 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D24183: A clang tool for changing surrouding namespaces of class/function definitions.
ioeric updated this revision to Diff 71126.
ioeric marked 13 inline comments as done.
ioeric added a comment.
Herald added a subscriber: mgorny.
- Minor cleanup.
https://reviews.llvm.org/D24183
Files:
CMakeLists.txt
change-namespace/CMakeLists.txt
change-namespace/ChangeNamespace.cpp
change-namespace/ChangeNamespace.h
change-namespace/tool/CMakeLists.txt
change-namespace/tool/ClangChangeNamespace.cpp
test/CMakeLists.txt
test/change-namespace/simple-move.cpp
unittests/CMakeLists.txt
unittests/change-namespace/CMakeLists.txt
unittests/change-namespace/ChangeNamespaceTests.cpp
Index: unittests/change-namespace/ChangeNamespaceTests.cpp
===
--- /dev/null
+++ unittests/change-namespace/ChangeNamespaceTests.cpp
@@ -0,0 +1,234 @@
+//===-- ChangeNamespaceTests.cpp - Change namespace unit tests ---*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "ChangeNamespace.h"
+#include "unittests/Tooling/RewriterTestContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Basic/FileManager.h"
+#include "clang/Basic/FileSystemOptions.h"
+#include "clang/Basic/VirtualFileSystem.h"
+#include "clang/Format/Format.h"
+#include "clang/Frontend/CompilerInstance.h"
+#include "clang/Frontend/PCHContainerOperations.h"
+#include "clang/Tooling/Refactoring.h"
+#include "clang/Tooling/Tooling.h"
+#include "llvm/ADT/IntrusiveRefCntPtr.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/MemoryBuffer.h"
+#include "gtest/gtest.h"
+#include
+#include
+#include
+
+namespace clang {
+namespace change_namespace {
+namespace {
+
+class ChangeNamespaceTest : public ::testing::Test {
+public:
+ std::string runChangeNamespaceOnCode(llvm::StringRef Code) {
+clang::RewriterTestContext Context;
+clang::FileID ID = Context.createInMemoryFile(FileName, Code);
+
+std::map FileToReplacements;
+change_namespace::ChangeNamespaceTool NamespaceTool(
+OldNamespace, NewNamespace, FilePattern, &FileToReplacements);
+ast_matchers::MatchFinder Finder;
+NamespaceTool.registerMatchers(&Finder);
+std::unique_ptr Factory =
+tooling::newFrontendActionFactory(&Finder);
+tooling::runToolOnCodeWithArgs(Factory->create(), Code, {"-std=c++11"},
+ FileName);
+formatAndApplyAllReplacements(FileToReplacements, Context.Rewrite);
+return format(Context.getRewrittenText(ID));
+ }
+
+ std::string format(llvm::StringRef Code) {
+tooling::Replacements Replaces = format::reformat(
+format::getLLVMStyle(), Code, {tooling::Range(0, Code.size())});
+auto ChangedCode = tooling::applyAllReplacements(Code, Replaces);
+EXPECT_TRUE(static_cast(ChangedCode));
+if (!ChangedCode) {
+ llvm::errs() << llvm::toString(ChangedCode.takeError());
+ return "";
+}
+return *ChangedCode;
+ }
+
+protected:
+ std::string FileName = "input.cc";
+ std::string OldNamespace = "na::nb";
+ std::string NewNamespace = "x::y";
+ std::string FilePattern = "input.cc";
+};
+
+TEST_F(ChangeNamespaceTest, NoMatchingNamespace) {
+ std::string Code = "namespace na {\n"
+ "namespace nx {\n"
+ "class A {};\n"
+ "} // namespace nx\n"
+ "} // namespace na\n";
+ std::string Expected = "namespace na {\n"
+ "namespace nx {\n"
+ "class A {};\n"
+ "} // namespace nx\n"
+ "} // namespace na\n";
+ EXPECT_EQ(format(Expected), runChangeNamespaceOnCode(Code));
+}
+
+TEST_F(ChangeNamespaceTest, SimpleMoveWithoutTypeRefs) {
+ std::string Code = "namespace na {\n"
+ "namespace nb {\n"
+ "class A {};\n"
+ "} // namespace nb\n"
+ "} // namespace na\n";
+ std::string Expected = "\n\n"
+ "namespace x {\n"
+ "namespace y {\n"
+ "class A {};\n"
+ "} // namespace y\n"
+ "} // namespace x\n";
+ EXPECT_EQ(format(Expected), runChangeNamespaceOnCode(Code));
+}
+
+TEST_F(ChangeNamespaceTest, SimpleMoveIntoAnotherNestedNamespace) {
+ NewNamespace = "na::nc";
+ std::string Code = "namespace na {\n"
+ "namespace nb {\n"
+ "class A {};\n"
+ "} // namespace nb\n"
+ "} // namespace na\n";
+ std::string Expected = "namespace na {\n"
+ "\n"
+ "namespace nc {\n"
+ "class A {};\n"
+ "} // namespace nc\n"
+
Re: [PATCH] D24183: A clang tool for changing surrouding namespaces of class/function definitions.
omtcyfz added inline comments. Comment at: change-namespace/ChangeNamespace.cpp:232 @@ +231,3 @@ +} + +// FIXME(ioeric): handle the following symbols: handle == "ioeric" here s/FIXME(ioeric)/FIXME Comment at: change-namespace/ChangeNamespace.cpp:336 @@ +335,3 @@ +// creates an `InsertForwardDeclaration` to insert the forward declaration back +// into the old namespace after moving code from the old namespace to the new +// namespace. Ah, I see. Thank you! https://reviews.llvm.org/D24183 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D24183: A clang tool for changing surrouding namespaces of class/function definitions.
ioeric updated this revision to Diff 70793.
ioeric marked 16 inline comments as done.
ioeric added a comment.
- Addressed reviewer comments.
https://reviews.llvm.org/D24183
Files:
CMakeLists.txt
change-namespace/CMakeLists.txt
change-namespace/ChangeNamespace.cpp
change-namespace/ChangeNamespace.h
change-namespace/tool/CMakeLists.txt
change-namespace/tool/ClangChangeNamespace.cpp
test/CMakeLists.txt
test/change-namespace/simple-move.cpp
unittests/CMakeLists.txt
unittests/change-namespace/CMakeLists.txt
unittests/change-namespace/ChangeNamespaceTests.cpp
Index: unittests/change-namespace/ChangeNamespaceTests.cpp
===
--- /dev/null
+++ unittests/change-namespace/ChangeNamespaceTests.cpp
@@ -0,0 +1,234 @@
+//===-- ChangeNamespaceTests.cpp - Change namespace unit tests ---*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "ChangeNamespace.h"
+#include "unittests/Tooling/RewriterTestContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Basic/FileManager.h"
+#include "clang/Basic/FileSystemOptions.h"
+#include "clang/Basic/VirtualFileSystem.h"
+#include "clang/Format/Format.h"
+#include "clang/Frontend/CompilerInstance.h"
+#include "clang/Frontend/PCHContainerOperations.h"
+#include "clang/Tooling/Refactoring.h"
+#include "clang/Tooling/Tooling.h"
+#include "llvm/ADT/IntrusiveRefCntPtr.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/MemoryBuffer.h"
+#include "gtest/gtest.h"
+#include
+#include
+#include
+
+namespace clang {
+namespace change_namespace {
+namespace {
+
+class ChangeNamespaceTest : public ::testing::Test {
+public:
+ std::string runChangeNamespaceOnCode(llvm::StringRef Code) {
+clang::RewriterTestContext Context;
+clang::FileID ID = Context.createInMemoryFile(FileName, Code);
+
+std::map FileToReplacements;
+change_namespace::ChangeNamespaceTool NamespaceTool(
+OldNamespace, NewNamespace, FilePattern, &FileToReplacements);
+ast_matchers::MatchFinder Finder;
+NamespaceTool.registerMatchers(&Finder);
+std::unique_ptr Factory =
+tooling::newFrontendActionFactory(&Finder);
+tooling::runToolOnCodeWithArgs(Factory->create(), Code, {"-std=c++11"},
+ FileName);
+formatAndApplyAllReplacements(FileToReplacements, Context.Rewrite);
+return format(Context.getRewrittenText(ID));
+ }
+
+ std::string format(llvm::StringRef Code) {
+tooling::Replacements Replaces = format::reformat(
+format::getLLVMStyle(), Code, {tooling::Range(0, Code.size())});
+auto ChangedCode = tooling::applyAllReplacements(Code, Replaces);
+EXPECT_TRUE(static_cast(ChangedCode));
+if (!ChangedCode) {
+ llvm::errs() << llvm::toString(ChangedCode.takeError());
+ return "";
+}
+return *ChangedCode;
+ }
+
+protected:
+ std::string FileName = "input.cc";
+ std::string OldNamespace = "na::nb";
+ std::string NewNamespace = "x::y";
+ std::string FilePattern = "input.cc";
+};
+
+TEST_F(ChangeNamespaceTest, NoMatchingNamespace) {
+ std::string Code = "namespace na {\n"
+ "namespace nx {\n"
+ "class A {};\n"
+ "} // namespace nx\n"
+ "} // namespace na\n";
+ std::string Expected = "namespace na {\n"
+ "namespace nx {\n"
+ "class A {};\n"
+ "} // namespace nx\n"
+ "} // namespace na\n";
+ EXPECT_EQ(format(Expected), runChangeNamespaceOnCode(Code));
+}
+
+TEST_F(ChangeNamespaceTest, SimpleMoveWithoutTypeRefs) {
+ std::string Code = "namespace na {\n"
+ "namespace nb {\n"
+ "class A {};\n"
+ "} // namespace nb\n"
+ "} // namespace na\n";
+ std::string Expected = "\n\n"
+ "namespace x {\n"
+ "namespace y {\n"
+ "class A {};\n"
+ "} // namespace y\n"
+ "} // namespace x\n";
+ EXPECT_EQ(format(Expected), runChangeNamespaceOnCode(Code));
+}
+
+TEST_F(ChangeNamespaceTest, SimpleMoveIntoAnotherNestedNamespace) {
+ NewNamespace = "na::nc";
+ std::string Code = "namespace na {\n"
+ "namespace nb {\n"
+ "class A {};\n"
+ "} // namespace nb\n"
+ "} // namespace na\n";
+ std::string Expected = "namespace na {\n"
+ "\n"
+ "namespace nc {\n"
+ "class A {};\n"
+ "} // namespace nc\n"
+ "} // na
Re: [PATCH] D24183: A clang tool for changing surrouding namespaces of class/function definitions.
ioeric added inline comments.
Comment at: change-namespace/ChangeNamespace.cpp:85
@@ +84,3 @@
+
+SourceLocation getStartOfNextLine(SourceLocation Loc, const SourceManager &SM,
+ const LangOptions &LangOpts) {
omtcyfz wrote:
> Wouldn't it be better of in some common interface?
>
> A couple of useful cases for other refactorings come to my mind. I.e.
> `getStartOfPreviousLine` would be nice to have there, too.
>
> For this patch, though, I think `FIXME` would be enough.
Acked.
Comment at: change-namespace/ChangeNamespace.cpp:124
@@ +123,3 @@
+// Adds a replacement `R` into `Replaces` or merges it into `Replaces` by
+// applying all existing Replaces first if there is conflict.
+void addOrMergeReplacement(const tooling::Replacement &R,
omtcyfz wrote:
> (probably not very much related to the patch and more out of curiosity)
>
> If there is a conflict, why does it get resolved by applying all existing
> `Replaces` first?
It's a bit complicated to explain...Please see the comment for
`tooling::Replacements`, specifically the `merge` function. Generally, the idea
is that conflicting replacements will be merged into a single one so that the
set of replacements never conflict.
Comment at: change-namespace/ChangeNamespace.cpp:139
@@ +138,3 @@
+ if (!Start.isValid() || !End.isValid()) {
+llvm::errs() << "start or end location were invalid\n";
+return tooling::Replacement();
omtcyfz wrote:
> Alex suggested using diagnostics instead of dumping stuff to `llvm::errs()`
> in D24224 and I think it looks way better. Actually, you can output locations
> with that, which makes error message more informative. It might be also
> easier to track down bugs with that info, rather than with raw error messages.
>
> That's just a nit, probably `FIXME` is enough.
>
> Same comment applies to the other `llvm::errs()` usages in this function.
Acked.
Comment at: change-namespace/ChangeNamespace.cpp:231
@@ +230,3 @@
+
+// FIXME(ioeric): handle the following symbols:
+// - Types in `UsingShadowDecl` (e.g. `using a::b::c;`) which are not matched
omtcyfz wrote:
> `FIXME` without handle here?
?
Comment at: change-namespace/ChangeNamespace.cpp:335
@@ +334,3 @@
+// into the old namespace after moving code from the old namespace to the new
+// namespace.
+void ChangeNamespaceTool::moveClassForwardDeclaration(
omtcyfz wrote:
> Slightly strange wording. I recall an offline discussion where you told about
> it, so I can understand that, but probably an example would be cool to have.
There is an example in the comment for the class. Also copied it here :)
Comment at: change-namespace/ChangeNamespace.cpp:381
@@ +380,3 @@
+ llvm::StringRef Postfix = OldNs;
+ bool Consumed = Postfix.consume_front(OldNamespace);
+ assert(Consumed);
omtcyfz wrote:
> Also, if you only use this for an assert, why not just pass
> `Postfix.consume_front(OldNamespace)` to `assert`?
I think it is not a good idea to put code with side-effect into `assert` since
assertion can be disabled.
https://reviews.llvm.org/D24183
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D24183: A clang tool for changing surrouding namespaces of class/function definitions.
omtcyfz added a comment. Also +R Alex if he has some time to take a look at the code. https://reviews.llvm.org/D24183 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D24183: A clang tool for changing surrouding namespaces of class/function definitions.
omtcyfz added a comment.
A round of mostly stylistic comments.
Comment at: change-namespace/ChangeNamespace.cpp:85
@@ +84,3 @@
+
+SourceLocation getStartOfNextLine(SourceLocation Loc, const SourceManager &SM,
+ const LangOptions &LangOpts) {
Wouldn't it be better of in some common interface?
A couple of useful cases for other refactorings come to my mind. I.e.
`getStartOfPreviousLine` would be nice to have there, too.
For this patch, though, I think `FIXME` would be enough.
Comment at: change-namespace/ChangeNamespace.cpp:111
@@ +110,3 @@
+
+// Returns a new replacement that is `R` with range that refers to
+// code after `Replaces` being applied.
A little confusing wording "a new replacement that is `R`". Here and later.
Comment at: change-namespace/ChangeNamespace.cpp:124
@@ +123,3 @@
+// Adds a replacement `R` into `Replaces` or merges it into `Replaces` by
+// applying all existing Replaces first if there is conflict.
+void addOrMergeReplacement(const tooling::Replacement &R,
(probably not very much related to the patch and more out of curiosity)
If there is a conflict, why does it get resolved by applying all existing
`Replaces` first?
Comment at: change-namespace/ChangeNamespace.cpp:139
@@ +138,3 @@
+ if (!Start.isValid() || !End.isValid()) {
+llvm::errs() << "start or end location were invalid\n";
+return tooling::Replacement();
Alex suggested using diagnostics instead of dumping stuff to `llvm::errs()` in
D24224 and I think it looks way better. Actually, you can output locations with
that, which makes error message more informative. It might be also easier to
track down bugs with that info, rather than with raw error messages.
That's just a nit, probably `FIXME` is enough.
Same comment applies to the other `llvm::errs()` usages in this function.
Comment at: change-namespace/ChangeNamespace.cpp:179
@@ +178,3 @@
+return DeclName;
+ auto UnqualifiedName = DeclNameSplitted.back();
+ while (true) {
Shouldn't this one be `const`?
Comment at: change-namespace/ChangeNamespace.cpp:184
@@ +183,3 @@
+ return DeclName;
+auto Prefix = NsName.substr(0, Pos - 1);
+if (DeclName.startswith(Prefix))
Same for `Prefix` and `Pos`.
Comment at: change-namespace/ChangeNamespace.cpp:199
@@ +198,3 @@
+Code = ("namespace " + NsSplitted.back() + " {\n" + Code +
+"} // namespace " + NsSplitted.back() + "\n")
+ .str();
Probably `"} // end namespace "` here, at least that's what [[
http://llvm.org/docs/CodingStandards.html#namespace-indentation | LLVM Coding
Standards ]] suggest.
Comment at: change-namespace/ChangeNamespace.cpp:231
@@ +230,3 @@
+
+// FIXME(ioeric): handle the following symbols:
+// - Types in `UsingShadowDecl` (e.g. `using a::b::c;`) which are not matched
`FIXME` without handle here?
Comment at: change-namespace/ChangeNamespace.cpp:307
@@ +306,3 @@
+ // retrieving offset and length from it.
+ auto R = createReplacement(Start, End, "", *Result.SourceManager);
+ MoveNamespace MoveNs;
`const` here, too?
Comment at: change-namespace/ChangeNamespace.cpp:335
@@ +334,3 @@
+// into the old namespace after moving code from the old namespace to the new
+// namespace.
+void ChangeNamespaceTool::moveClassForwardDeclaration(
Slightly strange wording. I recall an offline discussion where you told about
it, so I can understand that, but probably an example would be cool to have.
Comment at: change-namespace/ChangeNamespace.cpp:347
@@ +346,3 @@
+ // Delete the forward declaration from the code to be moved.
+ auto Deletion = createReplacement(Start, End, "", *Result.SourceManager);
+ addOrMergeReplacement(Deletion, &FileToReplacements[Deletion.getFilePath()]);
`const` here, too?
Comment at: change-namespace/ChangeNamespace.cpp:362
@@ +361,3 @@
+ assert(!NsDecl->decls_empty());
+ auto Insertion = createInsertion(NsDecl->decls_begin()->getLocStart(), Code,
+ *Result.SourceManager);
`const` here, too?
Comment at: change-namespace/ChangeNamespace.cpp:381
@@ +380,3 @@
+ llvm::StringRef Postfix = OldNs;
+ bool Consumed = Postfix.consume_front(OldNamespace);
+ assert(Consumed);
Also, if you only use this for an assert, why not just pass
`Postfix.consume_front(OldNamespace)` to `assert`?
Comment at: change-namespace/ChangeNamespace.cpp:382
@@ +381,3 @@
+ bool Consumed = Postfix.consume_front(OldNamespace);
+ assert(Consumed);
+ (void)Consumed;
`assert`s are even more cool when they contain informative
Re: [PATCH] D24183: A clang tool for changing surrouding namespaces of class/function definitions.
ioeric added a comment. Ping https://reviews.llvm.org/D24183 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D24183: A clang tool for changing surrouding namespaces of class/function definitions.
ioeric added inline comments.
Comment at: change-namespace/ChangeNamespace.cpp:125
@@ +124,3 @@
+// applying all existing Replaces first if there is conflict.
+void addOrMergeReplacement(const tooling::Replacement &R,
+ tooling::Replacements *Replaces) {
alexshap wrote:
> khm, this seems to be a recurring pattern (if i'm not mistaken),
> looks like the interface of tooling::Replacements can be changed/improved
> (although i don't know).
> Another (separate) observation is about duplicates. For example for
> replacements in headers we can get into if(Err) branch pretty frequently
> because the same replacement can be generated multiple times (if that header
> is included into several *.cpp files).
SGTM. An `addOrMerge()` interface for tooling::Replacements should be helpful.
Comment at: change-namespace/ChangeNamespace.cpp:481
@@ +480,3 @@
+ continue;
+}
+FileToReplacements[FilePath] = *CleanReplacements;
Added a `FallbackStyle`.
Comment at: change-namespace/tool/ClangChangeNamespace.cpp:105
@@ +104,3 @@
+outs() << "== " << File << " ==\n";
+Rewrite.getEditBuffer(ID).write(llvm::outs());
+outs() << "\n\n";
I'd like to keep this for now for better readability. Will change the format in
the future if necessary.
https://reviews.llvm.org/D24183
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D24183: A clang tool for changing surrouding namespaces of class/function definitions.
ioeric updated this revision to Diff 70588.
ioeric marked 9 inline comments as done.
ioeric added a comment.
Herald added a subscriber: beanz.
- Addressed reviewer comments.
https://reviews.llvm.org/D24183
Files:
CMakeLists.txt
change-namespace/CMakeLists.txt
change-namespace/ChangeNamespace.cpp
change-namespace/ChangeNamespace.h
change-namespace/tool/CMakeLists.txt
change-namespace/tool/ClangChangeNamespace.cpp
test/CMakeLists.txt
test/change-namespace/simple-move.cpp
unittests/CMakeLists.txt
unittests/change-namespace/CMakeLists.txt
unittests/change-namespace/ChangeNamespaceTests.cpp
Index: unittests/change-namespace/ChangeNamespaceTests.cpp
===
--- /dev/null
+++ unittests/change-namespace/ChangeNamespaceTests.cpp
@@ -0,0 +1,234 @@
+//===-- ChangeNamespaceTests.cpp - Change namespace unit tests ---*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "ChangeNamespace.h"
+#include "unittests/Tooling/RewriterTestContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Basic/FileManager.h"
+#include "clang/Basic/FileSystemOptions.h"
+#include "clang/Basic/VirtualFileSystem.h"
+#include "clang/Format/Format.h"
+#include "clang/Frontend/CompilerInstance.h"
+#include "clang/Frontend/PCHContainerOperations.h"
+#include "clang/Tooling/Refactoring.h"
+#include "clang/Tooling/Tooling.h"
+#include "llvm/ADT/IntrusiveRefCntPtr.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/MemoryBuffer.h"
+#include "gtest/gtest.h"
+#include
+#include
+#include
+
+namespace clang {
+namespace change_namespace {
+namespace {
+
+class ChangeNamespaceTest : public ::testing::Test {
+public:
+ std::string runChangeNamespaceOnCode(llvm::StringRef Code) {
+clang::RewriterTestContext Context;
+clang::FileID ID = Context.createInMemoryFile(FileName, Code);
+
+std::map FileToReplacements;
+change_namespace::ChangeNamespaceTool NamespaceTool(
+OldNamespace, NewNamespace, FilePattern, &FileToReplacements);
+ast_matchers::MatchFinder Finder;
+NamespaceTool.registerMatchers(&Finder);
+std::unique_ptr Factory =
+tooling::newFrontendActionFactory(&Finder);
+tooling::runToolOnCodeWithArgs(Factory->create(), Code, {"-std=c++11"},
+ FileName);
+formatAndApplyAllReplacements(FileToReplacements, Context.Rewrite);
+return format(Context.getRewrittenText(ID));
+ }
+
+ std::string format(llvm::StringRef Code) {
+tooling::Replacements Replaces = format::reformat(
+format::getLLVMStyle(), Code, {tooling::Range(0, Code.size())});
+auto ChangedCode = tooling::applyAllReplacements(Code, Replaces);
+EXPECT_TRUE(static_cast(ChangedCode));
+if (!ChangedCode) {
+ llvm::errs() << llvm::toString(ChangedCode.takeError());
+ return "";
+}
+return *ChangedCode;
+ }
+
+protected:
+ std::string FileName = "input.cc";
+ std::string OldNamespace = "na::nb";
+ std::string NewNamespace = "x::y";
+ std::string FilePattern = "input.cc";
+};
+
+TEST_F(ChangeNamespaceTest, NoMatchingNamespace) {
+ std::string Code = "namespace na {\n"
+ "namespace nx {\n"
+ "class A {};\n"
+ "} // namespace nx\n"
+ "} // namespace na\n";
+ std::string Expected = "namespace na {\n"
+ "namespace nx {\n"
+ "class A {};\n"
+ "} // namespace nx\n"
+ "} // namespace na\n";
+ EXPECT_EQ(format(Expected), runChangeNamespaceOnCode(Code));
+}
+
+TEST_F(ChangeNamespaceTest, SimpleMoveWithoutTypeRefs) {
+ std::string Code = "namespace na {\n"
+ "namespace nb {\n"
+ "class A {};\n"
+ "} // namespace nb\n"
+ "} // namespace na\n";
+ std::string Expected = "\n\n"
+ "namespace x {\n"
+ "namespace y {\n"
+ "class A {};\n"
+ "} // namespace y\n"
+ "} // namespace x\n";
+ EXPECT_EQ(format(Expected), runChangeNamespaceOnCode(Code));
+}
+
+TEST_F(ChangeNamespaceTest, SimpleMoveIntoAnotherNestedNamespace) {
+ NewNamespace = "na::nc";
+ std::string Code = "namespace na {\n"
+ "namespace nb {\n"
+ "class A {};\n"
+ "} // namespace nb\n"
+ "} // namespace na\n";
+ std::string Expected = "namespace na {\n"
+ "\n"
+ "namespace nc {\n"
+ "class A {};\n"
+ "} // namespace nc\n"
+
Re: [PATCH] D24183: A clang tool for changing surrouding namespaces of class/function definitions.
alexshap added inline comments.
Comment at: change-namespace/tool/ClangChangeNamespace.cpp:48
@@ +47,3 @@
+
+cl::opt OldNamespace("old_namespace", cl::desc("Old namespace."),
+ cl::cat(ChangeNamespaceCategory));
probably you need to add cl::Required
cl::opt OldNamespace("old_namespace", cl::Required,
cl::desc("Old namespace."),
cl::cat(ChangeNamespaceCategory));
and the same for NewNamespace.
https://reviews.llvm.org/D24183
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D24183: A clang tool for changing surrouding namespaces of class/function definitions.
alexshap added a subscriber: alexshap.
Comment at: change-namespace/ChangeNamespace.cpp:125
@@ +124,3 @@
+// applying all existing Replaces first if there is conflict.
+void addOrMergeReplacement(const tooling::Replacement &R,
+ tooling::Replacements *Replaces) {
khm, this seems to be a recurring pattern (if i'm not mistaken),
looks like the interface of tooling::Replacements can be changed/improved
(although i don't know).
Another (separate) observation is about duplicates. For example for
replacements in headers we can get into if(Err) branch pretty frequently
because the same replacement can be generated multiple times (if that header is
included into several *.cpp files).
https://reviews.llvm.org/D24183
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D24183: A clang tool for changing surrouding namespaces of class/function definitions.
ioeric added inline comments.
Comment at: change-namespace/ChangeNamespace.cpp:21
@@ +20,3 @@
+inline std::string formatNamespace(llvm::StringRef NS) {
+ (void)NS.ltrim(':');
+ return NS.str();
hokein wrote:
> this statement doesn't do the intended thing (It won't change `NS`). The
> result returned by `ltrim` is what you want here, I think.
Aha, no wonder! Thanks!
Comment at: change-namespace/ChangeNamespace.cpp:480
@@ +479,3 @@
+Replaces = Replaces.merge(NewReplacements);
+format::FormatStyle Style = format::getStyle("file", FilePath, "google");
+// Clean up old namespaces if there is nothing in it after moving.
hokein wrote:
> omtcyfz wrote:
> > omtcyfz wrote:
> > > By the way, shouldn't this one be "LLVM"?
> > Alternatively, it might be nice to provide an option to specify desired
> > formatting style.
> +1 on adding a `CodeStyle` command-line option.
Will do.
Comment at: unittests/change-namespace/ChangeNamespaceTests.cpp:49
@@ +48,3 @@
+formatAndApplyAllReplacements(FileToReplacements, Context.Rewrite);
+return format(Context.getRewrittenText(ID));
+ }
hokein wrote:
> Looks like `formatAndApplyAllReplacements` has already formatted the code,
> why do we need to format it again?
`formatAndApplyAllReplacements ` only formats around replacements. I added
`format` to format the whole file so that I don't need to get every white
space right in `Code` and `Expected`.
https://reviews.llvm.org/D24183
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D24183: A clang tool for changing surrouding namespaces of class/function definitions.
ioeric updated this revision to Diff 70465.
ioeric marked 10 inline comments as done.
ioeric added a comment.
- Addressed some review comments.
https://reviews.llvm.org/D24183
Files:
CMakeLists.txt
change-namespace/CMakeLists.txt
change-namespace/ChangeNamespace.cpp
change-namespace/ChangeNamespace.h
change-namespace/tool/CMakeLists.txt
change-namespace/tool/ClangChangeNamespace.cpp
test/CMakeLists.txt
test/change-namespace/simple-move.cpp
unittests/CMakeLists.txt
unittests/change-namespace/CMakeLists.txt
unittests/change-namespace/ChangeNamespaceTests.cpp
Index: unittests/change-namespace/ChangeNamespaceTests.cpp
===
--- /dev/null
+++ unittests/change-namespace/ChangeNamespaceTests.cpp
@@ -0,0 +1,234 @@
+//===-- ChangeNamespaceTests.cpp - Change namespace unit tests ---*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "ChangeNamespace.h"
+#include "unittests/Tooling/RewriterTestContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Basic/FileManager.h"
+#include "clang/Basic/FileSystemOptions.h"
+#include "clang/Basic/VirtualFileSystem.h"
+#include "clang/Format/Format.h"
+#include "clang/Frontend/CompilerInstance.h"
+#include "clang/Frontend/PCHContainerOperations.h"
+#include "clang/Tooling/Refactoring.h"
+#include "clang/Tooling/Tooling.h"
+#include "llvm/ADT/IntrusiveRefCntPtr.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/MemoryBuffer.h"
+#include "gtest/gtest.h"
+#include
+#include
+#include
+
+namespace clang {
+namespace change_namespace {
+namespace {
+
+class ChangeNamespaceTest : public ::testing::Test {
+public:
+ std::string runChangeNamespaceOnCode(llvm::StringRef Code) {
+clang::RewriterTestContext Context;
+clang::FileID ID = Context.createInMemoryFile(FileName, Code);
+
+std::map FileToReplacements;
+change_namespace::ChangeNamespaceTool NamespaceTool(
+OldNamespace, NewNamespace, FilePattern, &FileToReplacements);
+ast_matchers::MatchFinder Finder;
+NamespaceTool.registerMatchers(&Finder);
+std::unique_ptr Factory =
+tooling::newFrontendActionFactory(&Finder);
+tooling::runToolOnCodeWithArgs(Factory->create(), Code, {"-std=c++11"},
+ FileName);
+formatAndApplyAllReplacements(FileToReplacements, Context.Rewrite);
+return format(Context.getRewrittenText(ID));
+ }
+
+ std::string format(llvm::StringRef Code) {
+tooling::Replacements Replaces = format::reformat(
+format::getLLVMStyle(), Code, {tooling::Range(0, Code.size())});
+auto ChangedCode = tooling::applyAllReplacements(Code, Replaces);
+EXPECT_TRUE(static_cast(ChangedCode));
+if (!ChangedCode) {
+ llvm::errs() << llvm::toString(ChangedCode.takeError());
+ return "";
+}
+return *ChangedCode;
+ }
+
+protected:
+ std::string FileName = "input.cc";
+ std::string OldNamespace = "na::nb";
+ std::string NewNamespace = "x::y";
+ std::string FilePattern = "input.cc";
+};
+
+TEST_F(ChangeNamespaceTest, NoMatchingNamespace) {
+ std::string Code = "namespace na {\n"
+ "namespace nx {\n"
+ "class A {};\n"
+ "} // namespace nx\n"
+ "} // namespace na\n";
+ std::string Expected = "namespace na {\n"
+ "namespace nx {\n"
+ "class A {};\n"
+ "} // namespace nx\n"
+ "} // namespace na\n";
+ EXPECT_EQ(format(Expected), runChangeNamespaceOnCode(Code));
+}
+
+TEST_F(ChangeNamespaceTest, SimpleMoveWithoutTypeRefs) {
+ std::string Code = "namespace na {\n"
+ "namespace nb {\n"
+ "class A {};\n"
+ "} // namespace nb\n"
+ "} // namespace na\n";
+ std::string Expected = "\n\n"
+ "namespace x {\n"
+ "namespace y {\n"
+ "class A {};\n"
+ "} // namespace y\n"
+ "} // namespace x\n";
+ EXPECT_EQ(format(Expected), runChangeNamespaceOnCode(Code));
+}
+
+TEST_F(ChangeNamespaceTest, SimpleMoveIntoAnotherNestedNamespace) {
+ NewNamespace = "na::nc";
+ std::string Code = "namespace na {\n"
+ "namespace nb {\n"
+ "class A {};\n"
+ "} // namespace nb\n"
+ "} // namespace na\n";
+ std::string Expected = "namespace na {\n"
+ "\n"
+ "namespace nc {\n"
+ "class A {};\n"
+ "} // namespace nc\n"
+ "} //
Re: [PATCH] D24183: A clang tool for changing surrouding namespaces of class/function definitions.
hokein added a comment.
some initial comments.
Comment at: change-namespace/CMakeLists.txt:9
@@ +8,3 @@
+ LINK_LIBS
+ clangAST
+ clangBasic
I think `clangASTMatchers` is needed here.
Comment at: change-namespace/ChangeNamespace.cpp:21
@@ +20,3 @@
+inline std::string formatNamespace(llvm::StringRef NS) {
+ (void)NS.ltrim(':');
+ return NS.str();
this statement doesn't do the intended thing (It won't change `NS`). The result
returned by `ltrim` is what you want here, I think.
Comment at: change-namespace/ChangeNamespace.cpp:141
@@ +140,3 @@
+tooling::Replacement createReplacement(SourceLocation Start, SourceLocation
End,
+ const std::string &ReplacementText,
+ const SourceManager &SM) {
StringRef?
Comment at: change-namespace/ChangeNamespace.cpp:214
@@ +213,3 @@
+ChangeNamespaceTool::ChangeNamespaceTool(
+const std::string &OldNs, const std::string &NewNs,
+const std::string &FilePattern,
Use `StringRef`.
Comment at: change-namespace/ChangeNamespace.cpp:480
@@ +479,3 @@
+Replaces = Replaces.merge(NewReplacements);
+format::FormatStyle Style = format::getStyle("file", FilePath, "google");
+// Clean up old namespaces if there is nothing in it after moving.
omtcyfz wrote:
> omtcyfz wrote:
> > By the way, shouldn't this one be "LLVM"?
> Alternatively, it might be nice to provide an option to specify desired
> formatting style.
+1 on adding a `CodeStyle` command-line option.
Comment at: change-namespace/ChangeNamespace.h:67
@@ +66,3 @@
+
+ void FixUsingShadowDecl(const ast_matchers::MatchFinder::MatchResult &Result,
+ const UsingDecl *UsingDecl);
The first letter should be lower case.
Comment at: change-namespace/ChangeNamespace.h:70
@@ +69,3 @@
+
+ void ReplaceQualifiedSymbolInDeclContext(
+ const ast_matchers::MatchFinder::MatchResult &Result,
The same.
Comment at: change-namespace/tool/ClangChangeNamespace.cpp:95
@@ +94,3 @@
+
+ if (!formatAndApplyAllReplacements(Tool.getReplacements(), Rewrite)) {
+llvm::errs() << "Failed applying all replacements.\n";
Add a `CodeStyle` option?
Comment at: change-namespace/tool/ClangChangeNamespace.cpp:104
@@ +103,3 @@
+auto ID = Sources.translateFile(Entry);
+outs() << "== " << File << " ==\n";
+Rewrite.getEditBuffer(ID).write(llvm::outs());
Instead of printing results in a customized format, it makes more sense to
print it in a JSON format, like:
```
[
{ "FilePath": "XXX"
"SourceText": ""}
]
```
Comment at: test/change-namespace/simple-move.cpp:7
@@ +6,2 @@
+
+// RUN: clang-change-namespace -old_namespace "na::nb" -new_namespace "x::y"
--file_pattern ".*" %s -- | sed 's,// CHECK.*,,' | FileCheck %s
Usually, we put the test scripts at top of the test file, and use `CHECK-NEXT`
is more suitable in your case here?
Comment at: unittests/change-namespace/ChangeNamespaceTests.cpp:49
@@ +48,3 @@
+formatAndApplyAllReplacements(FileToReplacements, Context.Rewrite);
+return format(Context.getRewrittenText(ID));
+ }
Looks like `formatAndApplyAllReplacements` has already formatted the code, why
do we need to format it again?
https://reviews.llvm.org/D24183
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D24183: A clang tool for changing surrouding namespaces of class/function definitions.
omtcyfz added inline comments.
Comment at: change-namespace/ChangeNamespace.cpp:480
@@ +479,3 @@
+Replaces = Replaces.merge(NewReplacements);
+format::FormatStyle Style = format::getStyle("file", FilePath, "google");
+// Clean up old namespaces if there is nothing in it after moving.
omtcyfz wrote:
> By the way, shouldn't this one be "LLVM"?
Alternatively, it might be nice to provide an option to specify desired
formatting style.
https://reviews.llvm.org/D24183
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D24183: A clang tool for changing surrouding namespaces of class/function definitions.
omtcyfz added inline comments.
Comment at: change-namespace/ChangeNamespace.cpp:480
@@ +479,3 @@
+Replaces = Replaces.merge(NewReplacements);
+format::FormatStyle Style = format::getStyle("file", FilePath, "google");
+// Clean up old namespaces if there is nothing in it after moving.
By the way, shouldn't this one be "LLVM"?
https://reviews.llvm.org/D24183
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D24183: A clang tool for changing surrouding namespaces of class/function definitions.
ioeric added inline comments.
Comment at: change-namespace/ChangeNamespace.cpp:22
@@ +21,3 @@
+ (void)NS.ltrim(':');
+ return NS.str();
+}
Yes, it is added to please `LLVM_ATTRIBUTE_UNUSED_RESULT` of
`llvm::StringRef::ltrim`.
Comment at: change-namespace/ChangeNamespace.cpp:30
@@ +29,3 @@
+ std::string Result = Namespaces.front();
+ for (auto I = Namespaces.begin() + 1, E = Namespaces.end(); I != E; ++I) {
+Result += ("::" + *I).str();
omtcyfz wrote:
> Braces around single-line statement inside control flow statement body is
> occasionally not welcome in LLVM Codebase.
>
> There are actually both many braces around single-line statement inside cfs
> body here and many cfs's without braces around single-line body inside a
> single file, which isn't good in terms of consistency at least inside the
> project.
You are right! I missed those braces when converting the code from google style
:P Thanks for the catch!
Comment at: change-namespace/ChangeNamespace.cpp:105
@@ +104,3 @@
+ llvm::StringRef File = SM.getBufferData(LocInfo.first, &InvalidTemp);
+ if (InvalidTemp)
+return SourceLocation();
omtcyfz wrote:
> Shouldn't this throw an error instead?
An empty `SourceLocation` is invalid and often used to indicate error. Added
error handling at callers.
https://reviews.llvm.org/D24183
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D24183: A clang tool for changing surrouding namespaces of class/function definitions.
ioeric updated this revision to Diff 70308.
ioeric marked 4 inline comments as done.
ioeric added a comment.
- Addressed review comments.
https://reviews.llvm.org/D24183
Files:
CMakeLists.txt
change-namespace/CMakeLists.txt
change-namespace/ChangeNamespace.cpp
change-namespace/ChangeNamespace.h
change-namespace/tool/CMakeLists.txt
change-namespace/tool/ClangChangeNamespace.cpp
test/CMakeLists.txt
test/change-namespace/simple-move.cpp
unittests/CMakeLists.txt
unittests/change-namespace/CMakeLists.txt
unittests/change-namespace/ChangeNamespaceTests.cpp
Index: unittests/change-namespace/ChangeNamespaceTests.cpp
===
--- /dev/null
+++ unittests/change-namespace/ChangeNamespaceTests.cpp
@@ -0,0 +1,234 @@
+//===-- ChangeNamespaceTests.cpp - Change namespace unit tests ---*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "ChangeNamespace.h"
+#include "unittests/Tooling/RewriterTestContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Basic/FileManager.h"
+#include "clang/Basic/FileSystemOptions.h"
+#include "clang/Basic/VirtualFileSystem.h"
+#include "clang/Format/Format.h"
+#include "clang/Frontend/CompilerInstance.h"
+#include "clang/Frontend/PCHContainerOperations.h"
+#include "clang/Tooling/Refactoring.h"
+#include "clang/Tooling/Tooling.h"
+#include "llvm/ADT/IntrusiveRefCntPtr.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/MemoryBuffer.h"
+#include "gtest/gtest.h"
+#include
+#include
+#include
+
+namespace clang {
+namespace change_namespace {
+namespace {
+
+class ChangeNamespaceTest : public ::testing::Test {
+public:
+ std::string runChangeNamespaceOnCode(llvm::StringRef Code) {
+clang::RewriterTestContext Context;
+clang::FileID ID = Context.createInMemoryFile(FileName, Code);
+
+std::map FileToReplacements;
+change_namespace::ChangeNamespaceTool NamespaceTool(
+OldNamespace, NewNamespace, FilePattern, &FileToReplacements);
+ast_matchers::MatchFinder Finder;
+NamespaceTool.registerMatchers(&Finder);
+std::unique_ptr Factory =
+tooling::newFrontendActionFactory(&Finder);
+tooling::runToolOnCodeWithArgs(Factory->create(), Code, {"-std=c++11"},
+ FileName);
+formatAndApplyAllReplacements(FileToReplacements, Context.Rewrite);
+return format(Context.getRewrittenText(ID));
+ }
+
+ std::string format(llvm::StringRef Code) {
+tooling::Replacements Replaces = format::reformat(
+format::getLLVMStyle(), Code, {tooling::Range(0, Code.size())});
+auto ChangedCode = tooling::applyAllReplacements(Code, Replaces);
+EXPECT_TRUE(static_cast(ChangedCode));
+if (!ChangedCode) {
+ llvm::errs() << llvm::toString(ChangedCode.takeError());
+ return "";
+}
+return *ChangedCode;
+ }
+
+protected:
+ std::string FileName = "input.cc";
+ std::string OldNamespace = "na::nb";
+ std::string NewNamespace = "x::y";
+ std::string FilePattern = "input.cc";
+};
+
+TEST_F(ChangeNamespaceTest, NoMatchingNamespace) {
+ std::string Code = "namespace na {\n"
+ "namespace nx {\n"
+ "class A {};\n"
+ "} // namespace nx\n"
+ "} // namespace na\n";
+ std::string Expected = "namespace na {\n"
+ "namespace nx {\n"
+ "class A {};\n"
+ "} // namespace nx\n"
+ "} // namespace na\n";
+ EXPECT_EQ(format(Expected), runChangeNamespaceOnCode(Code));
+}
+
+TEST_F(ChangeNamespaceTest, SimpleMoveWithoutTypeRefs) {
+ std::string Code = "namespace na {\n"
+ "namespace nb {\n"
+ "class A {};\n"
+ "} // namespace nb\n"
+ "} // namespace na\n";
+ std::string Expected = "\n\n"
+ "namespace x {\n"
+ "namespace y {\n"
+ "class A {};\n"
+ "} // namespace y\n"
+ "} // namespace x\n";
+ EXPECT_EQ(format(Expected), runChangeNamespaceOnCode(Code));
+}
+
+TEST_F(ChangeNamespaceTest, SimpleMoveIntoAnotherNestedNamespace) {
+ NewNamespace = "na::nc";
+ std::string Code = "namespace na {\n"
+ "namespace nb {\n"
+ "class A {};\n"
+ "} // namespace nb\n"
+ "} // namespace na\n";
+ std::string Expected = "namespace na {\n"
+ "\n"
+ "namespace nc {\n"
+ "class A {};\n"
+ "} // namespace nc\n"
+ "} // names
Re: [PATCH] D24183: A clang tool for changing surrouding namespaces of class/function definitions.
omtcyfz added inline comments.
Comment at: change-namespace/ChangeNamespace.cpp:30
@@ +29,3 @@
+ std::string Result = Namespaces.front();
+ for (auto I = Namespaces.begin() + 1, E = Namespaces.end(); I != E; ++I) {
+Result += ("::" + *I).str();
Braces around single-line statement inside control flow statement body is
occasionally not welcome in LLVM Codebase.
There are actually both many braces around single-line statement inside cfs
body here and many cfs's without braces around single-line body inside a single
file, which isn't good in terms of consistency at least inside the project.
Comment at: change-namespace/ChangeNamespace.cpp:105
@@ +104,3 @@
+ llvm::StringRef File = SM.getBufferData(LocInfo.first, &InvalidTemp);
+ if (InvalidTemp)
+return SourceLocation();
Shouldn't this throw an error instead?
https://reviews.llvm.org/D24183
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D24183: A clang tool for changing surrouding namespaces of class/function definitions.
ioeric updated this revision to Diff 70173.
ioeric added a comment.
- removed unintended changes.
https://reviews.llvm.org/D24183
Files:
CMakeLists.txt
change-namespace/CMakeLists.txt
change-namespace/ChangeNamespace.cpp
change-namespace/ChangeNamespace.h
change-namespace/tool/CMakeLists.txt
change-namespace/tool/ClangChangeNamespace.cpp
test/CMakeLists.txt
test/change-namespace/simple-move.cpp
unittests/CMakeLists.txt
unittests/change-namespace/CMakeLists.txt
unittests/change-namespace/ChangeNamespaceTests.cpp
Index: unittests/change-namespace/ChangeNamespaceTests.cpp
===
--- /dev/null
+++ unittests/change-namespace/ChangeNamespaceTests.cpp
@@ -0,0 +1,234 @@
+//===-- ChangeNamespaceTests.cpp - Change namespace unit tests ---*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "ChangeNamespace.h"
+#include "unittests/Tooling/RewriterTestContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Basic/FileManager.h"
+#include "clang/Basic/FileSystemOptions.h"
+#include "clang/Basic/VirtualFileSystem.h"
+#include "clang/Format/Format.h"
+#include "clang/Frontend/CompilerInstance.h"
+#include "clang/Frontend/PCHContainerOperations.h"
+#include "clang/Tooling/Refactoring.h"
+#include "clang/Tooling/Tooling.h"
+#include "llvm/ADT/IntrusiveRefCntPtr.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/MemoryBuffer.h"
+#include "gtest/gtest.h"
+#include
+#include
+#include
+
+namespace clang {
+namespace change_namespace {
+namespace {
+
+class ChangeNamespaceTest : public ::testing::Test {
+public:
+ std::string runChangeNamespaceOnCode(llvm::StringRef Code) {
+clang::RewriterTestContext Context;
+clang::FileID ID = Context.createInMemoryFile(FileName, Code);
+
+std::map FileToReplacements;
+change_namespace::ChangeNamespaceTool NamespaceTool(
+OldNamespace, NewNamespace, FilePattern, &FileToReplacements);
+ast_matchers::MatchFinder Finder;
+NamespaceTool.registerMatchers(&Finder);
+std::unique_ptr Factory =
+tooling::newFrontendActionFactory(&Finder);
+tooling::runToolOnCodeWithArgs(Factory->create(), Code, {"-std=c++11"},
+ FileName);
+formatAndApplyAllReplacements(FileToReplacements, Context.Rewrite);
+return format(Context.getRewrittenText(ID));
+ }
+
+ std::string format(llvm::StringRef Code) {
+tooling::Replacements Replaces = format::reformat(
+format::getLLVMStyle(), Code, {tooling::Range(0, Code.size())});
+auto ChangedCode = tooling::applyAllReplacements(Code, Replaces);
+EXPECT_TRUE(static_cast(ChangedCode));
+if (!ChangedCode) {
+ llvm::errs() << llvm::toString(ChangedCode.takeError());
+ return "";
+}
+return *ChangedCode;
+ }
+
+protected:
+ std::string FileName = "input.cc";
+ std::string OldNamespace = "na::nb";
+ std::string NewNamespace = "x::y";
+ std::string FilePattern = "input.cc";
+};
+
+TEST_F(ChangeNamespaceTest, NoMatchingNamespace) {
+ std::string Code = "namespace na {\n"
+ "namespace nx {\n"
+ "class A {};\n"
+ "} // namespace nx\n"
+ "} // namespace na\n";
+ std::string Expected = "namespace na {\n"
+ "namespace nx {\n"
+ "class A {};\n"
+ "} // namespace nx\n"
+ "} // namespace na\n";
+ EXPECT_EQ(format(Expected), runChangeNamespaceOnCode(Code));
+}
+
+TEST_F(ChangeNamespaceTest, SimpleMoveWithoutTypeRefs) {
+ std::string Code = "namespace na {\n"
+ "namespace nb {\n"
+ "class A {};\n"
+ "} // namespace nb\n"
+ "} // namespace na\n";
+ std::string Expected = "\n\n"
+ "namespace x {\n"
+ "namespace y {\n"
+ "class A {};\n"
+ "} // namespace y\n"
+ "} // namespace x\n";
+ EXPECT_EQ(format(Expected), runChangeNamespaceOnCode(Code));
+}
+
+TEST_F(ChangeNamespaceTest, SimpleMoveIntoAnotherNestedNamespace) {
+ NewNamespace = "na::nc";
+ std::string Code = "namespace na {\n"
+ "namespace nb {\n"
+ "class A {};\n"
+ "} // namespace nb\n"
+ "} // namespace na\n";
+ std::string Expected = "namespace na {\n"
+ "\n"
+ "namespace nc {\n"
+ "class A {};\n"
+ "} // namespace nc\n"
+ "} // namespace na\n";
+ EXPECT_EQ(format(Expected
Re: [PATCH] D24183: A clang tool for changing surrouding namespaces of class/function definitions.
omtcyfz added inline comments.
Comment at: change-namespace/ChangeNamespace.cpp:21
@@ +20,3 @@
+inline std::string formatNamespace(llvm::StringRef NS) {
+ (void)NS.ltrim(':');
+ return NS.str();
is `(void)` intended here?
Comment at: change-namespace/ChangeNamespace.h:41
@@ +40,3 @@
+// } // x
+// TODO(ioeric): support moving typedef, enums across namespaces.
+class ChangeNamespaceTool : ast_matchers::MatchFinder::MatchCallback {
`FIXME`?
Repository:
rL LLVM
https://reviews.llvm.org/D24183
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D24183: A clang tool for changing surrouding namespaces of class/function definitions.
omtcyfz added a comment. In https://reviews.llvm.org/D24183#532876, @Eugene.Zelenko wrote: > Looks like clang-refactor idea should finally go live. https://reviews.llvm.org/D24192 up for review. Repository: rL LLVM https://reviews.llvm.org/D24183 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D24183: A clang tool for changing surrouding namespaces of class/function definitions.
Eugene.Zelenko added a subscriber: Eugene.Zelenko. Eugene.Zelenko added a comment. Looks like clang-refactor idea should finally go live. Repository: rL LLVM https://reviews.llvm.org/D24183 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D24183: A clang tool for changing surrouding namespaces of class/function definitions.
ioeric updated this revision to Diff 70155.
ioeric added a comment.
- Minor cleanup.
https://reviews.llvm.org/D24183
Files:
CMakeLists.txt
change-namespace/CMakeLists.txt
change-namespace/ChangeNamespace.cpp
change-namespace/ChangeNamespace.h
change-namespace/tool/CMakeLists.txt
change-namespace/tool/ClangChangeNamespace.cpp
clang-rename/tool/clang-rename.py
test/CMakeLists.txt
test/change-namespace/simple-move.cpp
unittests/CMakeLists.txt
unittests/change-namespace/CMakeLists.txt
unittests/change-namespace/ChangeNamespaceTests.cpp
Index: unittests/change-namespace/ChangeNamespaceTests.cpp
===
--- /dev/null
+++ unittests/change-namespace/ChangeNamespaceTests.cpp
@@ -0,0 +1,234 @@
+//===-- ChangeNamespaceTests.cpp - Change namespace unit tests ---*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "ChangeNamespace.h"
+#include "unittests/Tooling/RewriterTestContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Basic/FileManager.h"
+#include "clang/Basic/FileSystemOptions.h"
+#include "clang/Basic/VirtualFileSystem.h"
+#include "clang/Format/Format.h"
+#include "clang/Frontend/CompilerInstance.h"
+#include "clang/Frontend/PCHContainerOperations.h"
+#include "clang/Tooling/Refactoring.h"
+#include "clang/Tooling/Tooling.h"
+#include "llvm/ADT/IntrusiveRefCntPtr.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/MemoryBuffer.h"
+#include "gtest/gtest.h"
+#include
+#include
+#include
+
+namespace clang {
+namespace change_namespace {
+namespace {
+
+class ChangeNamespaceTest : public ::testing::Test {
+public:
+ std::string runChangeNamespaceOnCode(llvm::StringRef Code) {
+clang::RewriterTestContext Context;
+clang::FileID ID = Context.createInMemoryFile(FileName, Code);
+
+std::map FileToReplacements;
+change_namespace::ChangeNamespaceTool NamespaceTool(
+OldNamespace, NewNamespace, FilePattern, &FileToReplacements);
+ast_matchers::MatchFinder Finder;
+NamespaceTool.registerMatchers(&Finder);
+std::unique_ptr Factory =
+tooling::newFrontendActionFactory(&Finder);
+tooling::runToolOnCodeWithArgs(Factory->create(), Code, {"-std=c++11"},
+ FileName);
+formatAndApplyAllReplacements(FileToReplacements, Context.Rewrite);
+return format(Context.getRewrittenText(ID));
+ }
+
+ std::string format(llvm::StringRef Code) {
+tooling::Replacements Replaces = format::reformat(
+format::getLLVMStyle(), Code, {tooling::Range(0, Code.size())});
+auto ChangedCode = tooling::applyAllReplacements(Code, Replaces);
+EXPECT_TRUE(static_cast(ChangedCode));
+if (!ChangedCode) {
+ llvm::errs() << llvm::toString(ChangedCode.takeError());
+ return "";
+}
+return *ChangedCode;
+ }
+
+protected:
+ std::string FileName = "input.cc";
+ std::string OldNamespace = "na::nb";
+ std::string NewNamespace = "x::y";
+ std::string FilePattern = "input.cc";
+};
+
+TEST_F(ChangeNamespaceTest, NoMatchingNamespace) {
+ std::string Code = "namespace na {\n"
+ "namespace nx {\n"
+ "class A {};\n"
+ "} // namespace nx\n"
+ "} // namespace na\n";
+ std::string Expected = "namespace na {\n"
+ "namespace nx {\n"
+ "class A {};\n"
+ "} // namespace nx\n"
+ "} // namespace na\n";
+ EXPECT_EQ(format(Expected), runChangeNamespaceOnCode(Code));
+}
+
+TEST_F(ChangeNamespaceTest, SimpleMoveWithoutTypeRefs) {
+ std::string Code = "namespace na {\n"
+ "namespace nb {\n"
+ "class A {};\n"
+ "} // namespace nb\n"
+ "} // namespace na\n";
+ std::string Expected = "\n\n"
+ "namespace x {\n"
+ "namespace y {\n"
+ "class A {};\n"
+ "} // namespace y\n"
+ "} // namespace x\n";
+ EXPECT_EQ(format(Expected), runChangeNamespaceOnCode(Code));
+}
+
+TEST_F(ChangeNamespaceTest, SimpleMoveIntoAnotherNestedNamespace) {
+ NewNamespace = "na::nc";
+ std::string Code = "namespace na {\n"
+ "namespace nb {\n"
+ "class A {};\n"
+ "} // namespace nb\n"
+ "} // namespace na\n";
+ std::string Expected = "namespace na {\n"
+ "\n"
+ "namespace nc {\n"
+ "class A {};\n"
+ "} // namespace nc\n"
+ "} // namespace na\n";
+ EX
