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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
