[clang-move] Move comments which are associated with the moved class.

[ioeric]

This is great!

clang-move/ClangMove.cpp

70–74 ↗

(On Diff #73422)

Maybe:

return (SM->getLocForEndOfFile(LocInfo.first) == EndLoc) ? EndLoc : EndLoc.getLocWithOffset(1);

343 ↗

(On Diff #73422)

Looks like GetFullRange is called twice on the same Decl: one for creating insertion replacement and one for deletion replacement. This seems to be a duplicate.

[hokein]

Address review comments.

clang-move/ClangMove.cpp

343 ↗

(On Diff #73422)

Good point. Yeah, currently the GetFullRange is called twice. Have added a FIXME.

[ioeric]

lg

clang-tools-extra/trunk/clang-move/ClangMove.h

 namespace move {
 
 // FIXME: Make it support more types, e.g. function definitions.
 // Currently only support moving class definition.
 class ClangMoveTool : public ast_matchers::MatchFinder::MatchCallback {
 public:
   // Information about the declaration being moved.
   struct MovedDecl {
+    // FIXME: Replace Decl with SourceRange to get rid of calculating range for
+    // the Decl duplicately.
     const clang::NamedDecl *Decl = nullptr;
     clang::SourceManager *SM = nullptr;
     MovedDecl() = default;
     MovedDecl(const clang::NamedDecl *Decl, clang::SourceManager *SM)
         : Decl(Decl), SM(SM) {}
   };
 
   struct MoveDefinitionSpec {

clang-tools-extra/trunk/clang-move/ClangMove.cpp

if (const auto *FileEntry = SM.getFileEntryForID(SM.getFileID(HashLoc)))
                             FileEntry->getName(), SM);
   }
 
 private:
   const SourceManager &SM;
   ClangMoveTool *const MoveTool;
 };
 
+// Expand to get the end location of the line where the EndLoc of the given
+// Decl.
+SourceLocation
+getLocForEndOfDecl(const clang::Decl *D, const SourceManager *SM,
+                   const LangOptions &LangOpts = clang::LangOptions()) {
+  std::pair<FileID, unsigned> LocInfo = SM->getDecomposedLoc(D->getLocEnd());
+  // Try to load the file buffer.
+  bool InvalidTemp = false;
+  llvm::StringRef File = SM->getBufferData(LocInfo.first, &InvalidTemp);
+  if (InvalidTemp)
+    return SourceLocation();
+
+  const char *TokBegin = File.data() + LocInfo.second;
+  // Lex from the start of the given location.
+  Lexer Lex(SM->getLocForStartOfFile(LocInfo.first), LangOpts, File.begin(),
+            TokBegin, File.end());
+
+  llvm::SmallVector<char, 16> Line;
+  // FIXME: this is a bit hacky to get ReadToEndOfLine work.
+  Lex.setParsingPreprocessorDirective(true);
+  Lex.ReadToEndOfLine(&Line);
+  SourceLocation EndLoc  = D->getLocEnd().getLocWithOffset(Line.size());
+  // If we already reach EOF, just return the EOF SourceLocation;
+  // otherwise, move 1 offset ahead to include the trailing newline character
+  // '\n'.
+  return SM->getLocForEndOfFile(LocInfo.first) == EndLoc
+             ? EndLoc
+             : EndLoc.getLocWithOffset(1);
+}
+
+// Get full range of a Decl including the comments associated with it.
+clang::CharSourceRange
+GetFullRange(const clang::SourceManager *SM, const clang::Decl *D,
+             const clang::LangOptions &options = clang::LangOptions()) {
+  clang::SourceRange Full = D->getSourceRange();
+  Full.setEnd(getLocForEndOfDecl(D, SM));
+  // Expand to comments that are associated with the Decl.
+  if (const auto* Comment =
+          D->getASTContext().getRawCommentForDeclNoCache(D)) {
+    if (SM->isBeforeInTranslationUnit(Full.getEnd(), Comment->getLocEnd()))
+      Full.setEnd(Comment->getLocEnd());
+    // FIXME: Don't delete a preceding comment, if there are no other entities
+    // it could refer to.
+    if (SM->isBeforeInTranslationUnit(Comment->getLocStart(),
+                                      Full.getBegin()))
+      Full.setBegin(Comment->getLocStart());
+  }
+
+  return clang::CharSourceRange::getCharRange(Full);
+}
+
+std::string getDeclarationSourceText(const clang::Decl *D,
+                                     const clang::SourceManager *SM) {
+  llvm::StringRef SourceText = clang::Lexer::getSourceText(
+      GetFullRange(SM, D), *SM, clang::LangOptions());
+  return SourceText.str();
+}
+
 clang::tooling::Replacement
 getReplacementInChangedCode(const clang::tooling::Replacements &Replacements,
                             const clang::tooling::Replacement &Replacement) {
   unsigned Start = Replacements.getShiftedCodePosition(Replacement.getOffset());
   unsigned End = Replacements.getShiftedCodePosition(Replacement.getOffset() +
                                                      Replacement.getLength());
   return clang::tooling::Replacement(Replacement.getFilePath(), Start,
                                      End - Start,
for (const auto *Context = D->getDeclContext(); Context;
 
     if (const auto *ND = llvm::dyn_cast<clang::NamespaceDecl>(Context))
       Namespaces.push_back(ND->getName().str());
   }
   std::reverse(Namespaces.begin(), Namespaces.end());
   return Namespaces;
 }
 
-SourceLocation getLocForEndOfDecl(const clang::Decl *D,
-                                  const clang::SourceManager *SM) {
-  auto End = D->getLocEnd();
-  clang::SourceLocation AfterSemi = clang::Lexer::findLocationAfterToken(
-      End, clang::tok::semi, *SM, clang::LangOptions(),
-      /*SkipTrailingWhitespaceAndNewLine=*/true);
-  if (AfterSemi.isValid())
-    End = AfterSemi.getLocWithOffset(-1);
-  return End;
-}
-
-std::string getDeclarationSourceText(const clang::Decl *D,
-                                     const clang::SourceManager *SM) {
-  auto EndLoc = getLocForEndOfDecl(D, SM);
-  llvm::StringRef SourceText = clang::Lexer::getSourceText(
-      clang::CharSourceRange::getTokenRange(D->getLocStart(), EndLoc), *SM,
-      clang::LangOptions());
-  return SourceText.str() + "\n";
-}
-
 clang::tooling::Replacements
 createInsertedReplacements(const std::vector<std::string> &Includes,
                            const std::vector<ClangMoveTool::MovedDecl> &Decls,
                            llvm::StringRef FileName) {
   clang::tooling::Replacements InsertedReplacements;
 
   // Add #Includes.
   std::string AllIncludesString;
   // FIXME: Add header guard.
   for (const auto &Include : Includes)
     AllIncludesString += Include;
-  clang::tooling::Replacement InsertInclude(FileName, 0, 0, AllIncludesString);
+
+  if (!AllIncludesString.empty()) {
+    clang::tooling::Replacement InsertInclude(FileName, 0, 0,
+                                              AllIncludesString + "\n");
     addOrMergeReplacement(InsertInclude, &InsertedReplacements);
+  }
 
   // Add moved class definition and its related declarations. All declarations
   // in same namespace are grouped together.
   std::vector<std::string> CurrentNamespaces;
   for (const auto &MovedDecl : Decls) {
     std::vector<std::string> DeclNamespaces = GetNamespaces(MovedDecl.Decl);
     auto CurrentIt = CurrentNamespaces.begin();
     auto DeclIt = DeclNamespaces.begin();
for (const auto &MovedDecl : Decls) {
     }
     while (DeclIt != DeclNamespaces.end()) {
       clang::tooling::Replacement InsertedReplacement(
           FileName, 0, 0, "namespace " + *DeclIt + " {\n");
       addOrMergeReplacement(InsertedReplacement, &InsertedReplacements);
       ++DeclIt;
     }
 
-    // FIXME: consider moving comments of the moved declaration.
     clang::tooling::Replacement InsertedReplacement(
         FileName, 0, 0, getDeclarationSourceText(MovedDecl.Decl, MovedDecl.SM));
     addOrMergeReplacement(InsertedReplacement, &InsertedReplacements);
 
     CurrentNamespaces = std::move(NextNamespaces);
   }
   std::reverse(CurrentNamespaces.begin(), CurrentNamespaces.end());
   for (const auto &NS : CurrentNamespaces) {
HeaderIncludes.push_back(IncludeLine);
              AbsolutePath) {
     CCIncludes.push_back(IncludeLine);
   }
 }
 
 void ClangMoveTool::removeClassDefinitionInOldFiles() {
   for (const auto &MovedDecl : RemovedDecls) {
     const auto &SM = *MovedDecl.SM;
-    auto EndLoc = getLocForEndOfDecl(MovedDecl.Decl, &SM);
+    auto Range = GetFullRange(&SM, MovedDecl.Decl);
     clang::tooling::Replacement RemoveReplacement(
-        SM, clang::CharSourceRange::getTokenRange(MovedDecl.Decl->getLocStart(),
-                                                  EndLoc),
+        *MovedDecl.SM, clang::CharSourceRange::getCharRange(
+                           Range.getBegin(), Range.getEnd()),
         "");
     std::string FilePath = RemoveReplacement.getFilePath().str();
     addOrMergeReplacement(RemoveReplacement, &FileToReplacements[FilePath]);
 
     llvm::StringRef Code =
         SM.getBufferData(SM.getFileID(MovedDecl.Decl->getLocation()));
     format::FormatStyle Style =
         format::getStyle("file", FilePath, FallbackStyle);

clang-tools-extra/trunk/clang-move/tool/ClangMoveMain.cpp

 
 cl::opt<bool> Dump("dump_result",
                    cl::desc("Dump results in JSON format to stdout."),
                    cl::cat(ClangMoveCategory));
 
 } // namespace
 
 int main(int argc, const char **argv) {
-  tooling::CommonOptionsParser OptionsParser(argc, argv, ClangMoveCategory);
+  // Add "-fparse-all-comments" compile option to make clang parse all comments,
+  // otherwise, ordinary comments like "//" and "/*" won't get parsed (This is
+  // a bit of hacky).
+  std::vector<std::string> ExtraArgs( argv, argv + argc );
+  ExtraArgs.insert(ExtraArgs.begin()+1, "-extra-arg=-fparse-all-comments");
+  std::unique_ptr<const char *[]> RawExtraArgs(
+      new const char *[ExtraArgs.size()]);
+  for (size_t i = 0; i < ExtraArgs.size(); ++i)
+    RawExtraArgs[i] = ExtraArgs[i].c_str();
+  int Argc = argc + 1;
+  tooling::CommonOptionsParser OptionsParser(Argc, RawExtraArgs.get(),
+                                             ClangMoveCategory);
+
   tooling::RefactoringTool Tool(OptionsParser.getCompilations(),
                                 OptionsParser.getSourcePathList());
   move::ClangMoveTool::MoveDefinitionSpec Spec;
   Spec.Name = Name;
   Spec.OldHeader = OldHeader;
   Spec.NewHeader = NewHeader;
   Spec.OldCC = OldCC;
   Spec.NewCC = NewCC;

clang-tools-extra/trunk/unittests/clang-move/ClangMoveTests.cpp

 namespace move {
 namespace {
 
 const char TestHeaderName[] = "foo.h";
 
 const char TestCCName[] = "foo.cc";
 
 const char TestHeader[] = "namespace a {\n"
-                          "class C1;\n"
+                          "class C1; // test\n"
                           "namespace b {\n"
+                          "// This is a Foo class\n"
+                          "// which is used in\n"
+                          "// test.\n"
                           "class Foo {\n"
                           "public:\n"
                           "  void f();\n"
                           "\n"
                           "private:\n"
                           "  C1 *c1;\n"
                           "  static int b;\n"
-                          "};\n"
+                          "}; // abc\n"
                           "\n"
                           "class Foo2 {\n"
                           "public:\n"
                           "  int f();\n"
                           "};\n"
                           "} // namespace b\n"
                           "} // namespace a\n";
 
 const char TestCC[] = "#include \"foo.h\"\n"
                       "namespace a {\n"
                       "namespace b {\n"
                       "namespace {\n"
+                      "// comment1.\n"
                       "void f1() {}\n"
+                      "/// comment2.\n"
                       "int kConstInt1 = 0;\n"
                       "} // namespace\n"
                       "\n"
+                      "/* comment 3*/\n"
                       "static int kConstInt2 = 1;\n"
                       "\n"
+                      "/** comment4\n"
+                      "*/\n"
                       "static int help() {\n"
                       "  int a = 0;\n"
                       "  return a;\n"
                       "}\n"
                       "\n"
+                      "// comment5\n"
+                      "// comment5\n"
                       "void Foo::f() { f1(); }\n"
                       "\n"
+                      "/////////////\n"
+                      "// comment //\n"
+                      "/////////////\n"
                       "int Foo::b = 2;\n"
                       "int Foo2::f() {\n"
                       "  f1();\n"
                       "  return 1;\n"
                       "}\n"
                       "} // namespace b\n"
                       "} // namespace a\n";
 
 const char ExpectedTestHeader[] = "namespace a {\n"
-                                  "class C1;\n"
+                                  "class C1; // test\n"
                                   "namespace b {\n"
                                   "\n"
                                   "class Foo2 {\n"
                                   "public:\n"
                                   "  int f();\n"
                                   "};\n"
                                   "} // namespace b\n"
                                   "} // namespace a\n";
 
 const char ExpectedTestCC[] = "#include \"foo.h\"\n"
                               "namespace a {\n"
                               "namespace b {\n"
                               "namespace {\n"
+                              "// comment1.\n"
                               "void f1() {}\n"
+                              "/// comment2.\n"
                               "int kConstInt1 = 0;\n"
                               "} // namespace\n"
                               "\n"
+                              "/* comment 3*/\n"
                               "static int kConstInt2 = 1;\n"
                               "\n"
+                              "/** comment4\n"
+                              "*/\n"
                               "static int help() {\n"
                               "  int a = 0;\n"
                               "  return a;\n"
                               "}\n"
                               "\n"
                               "int Foo2::f() {\n"
                               "  f1();\n"
                               "  return 1;\n"
                               "}\n"
                               "} // namespace b\n"
                               "} // namespace a\n";
 
 const char ExpectedNewHeader[] = "namespace a {\n"
-                                 "class C1;\n"
+                                 "class C1; // test\n"
                                  "namespace b {\n"
+                                 "// This is a Foo class\n"
+                                 "// which is used in\n"
+                                 "// test.\n"
                                  "class Foo {\n"
                                  "public:\n"
                                  "  void f();\n"
                                  "\n"
                                  "private:\n"
                                  "  C1 *c1;\n"
                                  "  static int b;\n"
-                                 "};\n"
+                                 "}; // abc\n"
                                  "} // namespace b\n"
                                  "} // namespace a\n";
 
 const char ExpectedNewCC[] = "namespace a {\n"
                              "namespace b {\n"
                              "namespace {\n"
+                             "// comment1.\n"
                              "void f1() {}\n"
+                             "/// comment2.\n"
                              "int kConstInt1 = 0;\n"
                              "} // namespace\n"
+                             "/* comment 3*/\n"
                              "static int kConstInt2 = 1;\n"
+                             "/** comment4\n"
+                             "*/\n"
                              "static int help() {\n"
                              "  int a = 0;\n"
                              "  return a;\n"
                              "}\n"
+                             "// comment5\n"
+                             "// comment5\n"
                              "void Foo::f() { f1(); }\n"
+                             "/////////////\n"
+                             "// comment //\n"
+                             "/////////////\n"
                              "int Foo::b = 2;\n"
                              "} // namespace b\n"
                              "} // namespace a\n";
 
 std::map<std::string, std::string>
 runClangMoveOnCode(const move::ClangMoveTool::MoveDefinitionSpec &Spec) {
   clang::RewriterTestContext Context;
 
runClangMoveOnCode(const move::ClangMoveTool::MoveDefinitionSpec &Spec) {
   llvm::SmallString<128> InitialDirectory;
   std::error_code EC = llvm::sys::fs::current_path(InitialDirectory);
   assert(!EC);
   (void)EC;
   auto Factory = llvm::make_unique<clang::move::ClangMoveActionFactory>(
       Spec, FileToReplacements, InitialDirectory.str(), "LLVM");
 
   tooling::runToolOnCodeWithArgs(
-      Factory->create(), TestCC, {"-std=c++11"}, TestCCName, "clang-move",
-      std::make_shared<PCHContainerOperations>(), FileToSourceText);
+      Factory->create(), TestCC, {"-std=c++11", "-fparse-all-comments"},
+      TestCCName, "clang-move", std::make_shared<PCHContainerOperations>(),
+      FileToSourceText);
   formatAndApplyAllReplacements(FileToReplacements, Context.Rewrite, "llvm");
   // The Key is file name, value is the new code after moving the class.
   std::map<std::string, std::string> Results;
   for (const auto &It : FileToReplacements) {
     StringRef FilePath = It.first;
     Results[FilePath] = Context.getRewrittenText(FileToFileID[FilePath]);
   }
   return Results;
 }
 
 TEST(ClangMove, MoveHeaderAndCC) {
   move::ClangMoveTool::MoveDefinitionSpec Spec;
   Spec.Name = "a::b::Foo";
   Spec.OldHeader = "foo.h";
   Spec.OldCC = "foo.cc";
   Spec.NewHeader = "new_foo.h";
   Spec.NewCC = "new_foo.cc";
-  std::string ExpectedHeader = "#include \"" + Spec.NewHeader + "\"\n";
+  std::string ExpectedHeader = "#include \"" + Spec.NewHeader + "\"\n\n";
   auto Results = runClangMoveOnCode(Spec);
   EXPECT_EQ(ExpectedTestHeader, Results[Spec.OldHeader]);
   EXPECT_EQ(ExpectedTestCC, Results[Spec.OldCC]);
   EXPECT_EQ(ExpectedNewHeader, Results[Spec.NewHeader]);
   EXPECT_EQ(ExpectedHeader + ExpectedNewCC, Results[Spec.NewCC]);
 }
 
 TEST(ClangMove, MoveHeaderOnly) {
   move::ClangMoveTool::MoveDefinitionSpec Spec;
   Spec.Name = "a::b::Foo";
   Spec.OldHeader = "foo.h";
   Spec.NewHeader = "new_foo.h";
   auto Results = runClangMoveOnCode(Spec);
   EXPECT_EQ(2u, Results.size());
   EXPECT_EQ(ExpectedTestHeader, Results[Spec.OldHeader]);
   EXPECT_EQ(ExpectedNewHeader, Results[Spec.NewHeader]);
 }
 
 TEST(ClangMove, MoveCCOnly) {
   move::ClangMoveTool::MoveDefinitionSpec Spec;
   Spec.Name = "a::b::Foo";
   Spec.OldCC = "foo.cc";
   Spec.NewCC = "new_foo.cc";
-  std::string ExpectedHeader = "#include \"foo.h\"\n";
+  std::string ExpectedHeader = "#include \"foo.h\"\n\n";
   auto Results = runClangMoveOnCode(Spec);
   EXPECT_EQ(2u, Results.size());
   EXPECT_EQ(ExpectedTestCC, Results[Spec.OldCC]);
   EXPECT_EQ(ExpectedHeader + ExpectedNewCC, Results[Spec.NewCC]);
 }
 
 TEST(ClangMove, MoveNonExistClass) {
   move::ClangMoveTool::MoveDefinitionSpec Spec;