ioeric added a comment.
NIce! Some initial comments.
================
Comment at: clang-move/ClangMove.cpp:38
@@ +37,3 @@
+ const clang::Module * /*Imported*/) override {
+ if (const auto *FileEntry = SM.getFileEntryForID(SM.getFileID(HashLoc))) {
+ if (IsAngled) {
----------------
Might want to comment on how #include "old_header" is handled here?
================
Comment at: clang-move/ClangMove.cpp:39
@@ +38,3 @@
+ if (const auto *FileEntry = SM.getFileEntryForID(SM.getFileID(HashLoc))) {
+ if (IsAngled) {
+ MoveTool->addIncludes("#include <" + FileName.str() + ">\n",
----------------
Braces not needed.
================
Comment at: clang-move/ClangMove.cpp:103
@@ +102,3 @@
+ const clang::SourceManager *SM) {
+ // Gets the ending ";".
+ auto EndLoc = clang::Lexer::getLocForEndOfToken(D->getLocEnd(), 0, *SM,
----------------
Consider the code below to also include trailing comment.
clang::SourceLocation after_semi = clang::Lexer::findLocationAfterToken(
D->getLocEnd, clang::tok::semi, *SM,
result.Context->getLangOpts(),
/*SkipTrailingWhitespaceAndNewLine=*/true);
================
Comment at: clang-move/ClangMove.cpp:113
@@ +112,3 @@
+clang::tooling::Replacements
+createInsertedReplacements(const std::vector<std::string> &Includes,
+ const std::vector<ClangMoveTool::MovedDecl> &Decls,
----------------
Intuitively, it would be easier and more efficient if you construct the code as
a string and then insert the code with one replacement.
================
Comment at: clang-move/ClangMove.cpp:119
@@ +118,3 @@
+ // Add #Includes.
+ // FIXME: Filter out the old_header.h.
+ for (const auto &Include : Includes) {
----------------
FIXME: add header guard for new header file.
================
Comment at: clang-move/ClangMove.cpp:125
@@ +124,3 @@
+
+ // Add moved class definition and its related declarations. All declarations
+ // in same namespace are grouped together.
----------------
If two declarations in the same namespace are not consecutive in the vector
(i.e. there is a decl in a different ns between them), will they be put into
two canonical namespace blocks?
================
Comment at: clang-move/ClangMove.cpp:129
@@ +128,3 @@
+ for (const auto &MovedDecl : Decls) {
+ std::vector<std::string> DeclNamespaces = GetNamespaces(MovedDecl.Decl);
+ auto CurrentIt = CurrentNamespaces.begin();
----------------
Is it possible to restrict all `MovedDecl.Decl` to NamedDecl so that we can use
`getQualifiedNameAsString` instead of having a second implementation of
retrieving namespaces.
Also how is anonymous namespace handled here?
================
Comment at: clang-move/ClangMove.cpp:157
@@ +156,3 @@
+
+ clang::tooling::Replacement InsertedReplacement(
+ FileName, 0, 0, getDeclarationSourceText(MovedDecl.Decl,
MovedDecl.SM));
----------------
FIXME: also consider comments for the declaration.
================
Comment at: clang-move/ClangMove.cpp:199
@@ +198,3 @@
+
+ // Match functions defined in anonymous namespace.
+ Finder->addMatcher(
----------------
What about static functions?
================
Comment at: clang-move/ClangMove.cpp:210
@@ +209,3 @@
+ this);
+}
+
----------------
FIXME: support out-of-line member variable initializations etc? Or is it
already covered?
================
Comment at: clang-move/ClangMove.cpp:240
@@ +239,3 @@
+ llvm::StringRef FileName) {
+ if (!Spec.OldHeader.empty() && FileName.endswith(Spec.OldHeader)) {
+ HeaderIncludes.push_back(IncludeLine.str());
----------------
No braces.
Also, I am worrying if `endswith` is a safe way to compare files. For example,
what if `FileName` is relative path?
================
Comment at: clang-move/ClangMove.cpp:254
@@ +253,3 @@
+ MovedDecl.Decl->getLocEnd(), 0, *MovedDecl.SM, clang::LangOptions());
+ clang::tooling::Replacement RemovedReplacement(
+ *MovedDecl.SM, clang::CharSourceRange::getTokenRange(
----------------
nit: `RemovedReplacement` sounds like the replacement itself is being removed.
`RemoveReplacement` would be better IMO.
================
Comment at: clang-move/ClangMove.h:39
@@ +38,3 @@
+ struct MoveDefinitionSpec {
+ std::string Name;
+ std::string OldHeader;
----------------
Should the `Name` be fully qualified?
================
Comment at: clang-move/ClangMove.h:52
@@ +51,3 @@
+
+ void registerMatchers(clang::ast_matchers::MatchFinder *match_finder);
+
----------------
Variable style. Here and some more below.
================
Comment at: clang-move/ClangMove.h:59
@@ +58,3 @@
+
+ void addIncludes(llvm::StringRef include_line, llvm::StringRef file_name);
+
----------------
A comment for what this function does would be appreciated.
================
Comment at: clang-move/ClangMove.h:65
@@ +64,3 @@
+
+ const MoveDefinitionSpec &Spec;
+ // The Key is file path, value is the replacements being applied to the file.
----------------
Is there any reason why you made this a reference?
================
Comment at: clang-move/tool/ClangMoveMain.cpp:102
@@ +101,3 @@
+ for (const auto &it : Tool.getReplacements())
+ Files.insert(it.first);
+ auto WriteToJson = [&](llvm::raw_ostream& OS) {
----------------
Indentation.
https://reviews.llvm.org/D24243
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
