[PATCH] D24243: [clang-move] A prototype tool for moving class definition to new file.

[Eric Liu via cfe-commits]

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