[clang-move] Add header guard for the new header.

[hokein]

Summary

The header guard generated by clang-move isn't always a perfect
style, just avoid getting the header included multiple times during
compiling period.

Also, we can use llvm-Header-guard clang-tidy check to correct the guard
automatically.

[ioeric]

Lg

clang-move/ClangMove.cpp

264

Might not quite related to this patch:

Considering how tooling::Replacements::add(...) is implemented now, it's quite inefficient to use addOrMergeReplacement in you case since almost all replacements will conflict, which would require expensive conflict-resolving merge. I think it would be cleaner and easier if you just concatenate all Decls and create a single insertion replacement instead of keeping merging replacements at offset 0.

[hokein]

clang-move/ClangMove.cpp

264

Good point. Yeah, the function only inserts new code to the new file. Make sense to only construct a single Replacement of the concatenated string. Will do in a follow-up.

clang-move/ClangMove.cpp

std::vector<std::string> GetNamespaces(const clang::Decl *D) {
   }
   std::reverse(Namespaces.begin(), Namespaces.end());
   return Namespaces;
 }
 
 clang::tooling::Replacements
 createInsertedReplacements(const std::vector<std::string> &Includes,
                            const std::vector<ClangMoveTool::MovedDecl> &Decls,
-                           llvm::StringRef FileName) {
+                           llvm::StringRef FileName,
+                           bool IsHeader = false) {
   clang::tooling::Replacements InsertedReplacements;
+  std::string GuardName(FileName);
+  if (IsHeader) {
+    std::replace(GuardName.begin(), GuardName.end(), '/', '_');
+    std::replace(GuardName.begin(), GuardName.end(), '.', '_');
+    std::replace(GuardName.begin(), GuardName.end(), '-', '_');
+
+    GuardName = StringRef(GuardName).upper();
+    std::string HeaderGuard = "#ifndef " + GuardName + "\n";
+    HeaderGuard += "#define " + GuardName + "\n";
+    clang::tooling::Replacement HeaderGuardInclude(FileName, 0, 0,
+                                                   HeaderGuard);
+    addOrMergeReplacement(HeaderGuardInclude, &InsertedReplacements);
+  }
 
   // Add #Includes.
   std::string AllIncludesString;
   // FIXME: Add header guard.
   for (const auto &Include : Includes)
     AllIncludesString += Include;
 
   if (!AllIncludesString.empty()) {
for (const auto &MovedDecl : Decls) {
     CurrentNamespaces = std::move(NextNamespaces);
   }
   std::reverse(CurrentNamespaces.begin(), CurrentNamespaces.end());
   for (const auto &NS : CurrentNamespaces) {
     clang::tooling::Replacement InsertedReplacement(
         FileName, 0, 0, "} // namespace " + NS + "\n");
     addOrMergeReplacement(InsertedReplacement, &InsertedReplacements);
   }
+
+  if (IsHeader) {
+    clang::tooling::Replacement HeaderGuardEnd(FileName, 0, 0,
+                                               "#endif // " + GuardName + "\n");
+    addOrMergeReplacement(HeaderGuardEnd, &InsertedReplacements);
+  }
   return InsertedReplacements;
 }
 
 } // namespace
 
 std::unique_ptr<clang::ASTConsumer>
 ClangMoveAction::CreateASTConsumer(clang::CompilerInstance &Compiler,
                                    StringRef /*InFile*/) {
if (isInHeaderFile(*MovedDecl.SM, MovedDecl.Decl, OriginalRunningDirectory,
                        Spec.OldHeader))
       NewHeaderDecls.push_back(MovedDecl);
     else
       NewCCDecls.push_back(MovedDecl);
   }
 
   if (!Spec.NewHeader.empty())
     FileToReplacements[Spec.NewHeader] = createInsertedReplacements(
-        HeaderIncludes, NewHeaderDecls, Spec.NewHeader);
+        HeaderIncludes, NewHeaderDecls, Spec.NewHeader, /*IsHeader=*/true);
   if (!Spec.NewCC.empty())
     FileToReplacements[Spec.NewCC] =
         createInsertedReplacements(CCIncludes, NewCCDecls, Spec.NewCC);
 }
 
 void ClangMoveTool::onEndOfTranslationUnit() {
   if (RemovedDecls.empty())
     return;
   removeClassDefinitionInOldFiles();
   moveClassDefinitionToNewFiles();
 }
 
 } // namespace move
 } // namespace clang

test/clang-move/move-class.cpp

 // RUN: cp %S/Inputs/test.cpp %T/clang-move/src
 // RUN: cd %T/clang-move/build
 // RUN: clang-move -names="a::Foo" -new_cc=%T/clang-move/new_test.cpp -new_header=%T/clang-move/new_test.h -old_cc=%T/clang-move/src/test.cpp -old_header=%T/clang-move/include/test.h %T/clang-move/src/test.cpp
 // RUN: FileCheck -input-file=%T/clang-move/new_test.cpp -check-prefix=CHECK-NEW-TEST-CPP %s
 // RUN: FileCheck -input-file=%T/clang-move/new_test.h -check-prefix=CHECK-NEW-TEST-H %s
 // RUN: FileCheck -input-file=%T/clang-move/src/test.cpp -check-prefix=CHECK-OLD-TEST-CPP %s
 // RUN: FileCheck -input-file=%T/clang-move/include/test.h %s -implicit-check-not='{{namespace.*}}'
 //
+//
+// CHECK-NEW-TEST-H: #ifndef {{.*}}CLANG_MOVE_NEW_TEST_H
+// CHECK-NEW-TEST-H: #define {{.*}}CLANG_MOVE_NEW_TEST_H
 // CHECK-NEW-TEST-H: namespace a {
 // CHECK-NEW-TEST-H: class Foo {
 // CHECK-NEW-TEST-H: public:
 // CHECK-NEW-TEST-H:   int f();
 // CHECK-NEW-TEST-H:   int f2(int a, int b);
 // CHECK-NEW-TEST-H: };
 // CHECK-NEW-TEST-H: } // namespace a
+// CHECK-NEW-TEST-H: #endif // {{.*}}CLANG_MOVE_NEW_TEST_H
 //
 // CHECK-NEW-TEST-CPP: #include "{{.*}}new_test.h"
 // CHECK-NEW-TEST-CPP: #include "test2.h"
 // CHECK-NEW-TEST-CPP: namespace a {
 // CHECK-NEW-TEST-CPP: int Foo::f() { return 0; }
 // CHECK-NEW-TEST-CPP: int Foo::f2(int a, int b) { return a + b; }
 // CHECK-NEW-TEST-CPP: } // namespace a
 //
 // CHECK-OLD-TEST-CPP: #include "test.h"
 // CHECK-OLD-TEST-CPP: #include "test2.h"

test/clang-move/move-multiple-classes.cpp

 
 // CHECK-OLD-TEST-CPP: #include "{{.*}}multiple_class_test.h"
 // CHECK-OLD-TEST-CPP: namespace c {
 // CHECK-OLD-TEST-CPP: int NoMove::f() {
 // CHECK-OLD-TEST-CPP:   return 0;
 // CHECK-OLD-TEST-CPP: }
 // CHECK-OLD-TEST-CPP: } // namespace c
 
+// CHECK-NEW-TEST-H: #ifndef {{.*}}NEW_MULTIPLE_CLASS_TEST_H
+// CHECK-NEW-TEST-H: #define {{.*}}NEW_MULTIPLE_CLASS_TEST_H
 // CHECK-NEW-TEST-H: namespace a {
 // CHECK-NEW-TEST-H: class Move1 {
 // CHECK-NEW-TEST-H: public:
 // CHECK-NEW-TEST-H:   int f();
 // CHECK-NEW-TEST-H: };
 // CHECK-NEW-TEST-H: } // namespace a
 // CHECK-NEW-TEST-H: namespace b {
 // CHECK-NEW-TEST-H: class Move2 {

 // CHECK-NEW-TEST-H: public:
 // CHECK-NEW-TEST-H:   class Nested {
 // CHECK-NEW-TEST-H:     int f();
 // CHECK-NEW-TEST-H:     static int b;
 // CHECK-NEW-TEST-H:   };
 // CHECK-NEW-TEST-H:   static int a;
 // CHECK-NEW-TEST-H: };
 // CHECK-NEW-TEST-H: } // namespace c
+// CHECK-NEW-TEST-H: #endif // {{.*}}NEW_MULTIPLE_CLASS_TEST_H
 
 // CHECK-NEW-TEST-CPP: #include "{{.*}}new_multiple_class_test.h"
 // CHECK-NEW-TEST-CPP: namespace a {
 // CHECK-NEW-TEST-CPP: int Move1::f() { return 0; }
 // CHECK-NEW-TEST-CPP: } // namespace a
 // CHECK-NEW-TEST-CPP: namespace b {
 // CHECK-NEW-TEST-CPP: int Move2::f() { return 0; }
 // CHECK-NEW-TEST-CPP: } // namespace b
 // CHECK-NEW-TEST-CPP: namespace c {
 // CHECK-NEW-TEST-CPP: int Move3::f() { return 0; }
 // CHECK-NEW-TEST-CPP: int Move4::f() { return 0; }
 // CHECK-NEW-TEST-CPP: int EnclosingMove5::a = 1;
 // CHECK-NEW-TEST-CPP: int EnclosingMove5::Nested::f() { return 0; }
 // CHECK-NEW-TEST-CPP: int EnclosingMove5::Nested::b = 1;
 // CHECK-NEW-TEST-CPP: } // namespace c

unittests/clang-move/ClangMoveTests.cpp

const char ExpectedTestCC[] = "#include \"foo.h\"\n"
                               "\n"
                               "int Foo2::f() {\n"
                               "  f1();\n"
                               "  return 1;\n"
                               "}\n"
                               "} // namespace b\n"
                               "} // namespace a\n";
 
-const char ExpectedNewHeader[] = "namespace a {\n"
+const char ExpectedNewHeader[] = "#ifndef NEW_FOO_H\n"
+                                 "#define NEW_FOO_H\n"
+                                 "namespace a {\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"
                                  "}; // abc\n"
                                  "} // namespace b\n"
-                                 "} // namespace a\n";
+                                 "} // namespace a\n"
+                                 "#endif // NEW_FOO_H\n";
 
 const char ExpectedNewCC[] = "namespace a {\n"
                              "namespace b {\n"
                              "namespace {\n"
                              "// comment1.\n"
                              "void f1() {}\n"
                              "/// comment2.\n"
                              "int kConstInt1 = 0;\n"