[PATCH] D24183: A clang tool for changing surrouding namespaces of class/function definitions.

[Haojian Wu via cfe-commits]

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<std::string, tooling::Replacements> *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