[clang-format] make header guard identification stricter in header insertion.

[ioeric]

Summary

don't just identify first #define as header guard anymore. Use #ifndef
#define to tell if a #define belongs to header guard.

[ioeric]

• Use iterator looping to simplify code.

[djasper]

lib/Format/Format.cpp

1472

I think there is too much in the regex group here. If there are trailing comments on the #ifndef or #define lines, this should still work. Also, there might actually be empty lines or comment lines in-between the two. I would for now just check that there:

• Is no non-comment line before the first #ifndef
• That there is a #ifndef
• That the next non-comment line after the #ifndef is a #define

We can refine more if necessary.

I think we should be conservative here as this should only really ever affect .cc files (or else the header guard would be there and identified correctly) that don't have any #includes (or put them after unrelated #defines). So, we have to weigh this case being so rare against incorrectly identifying weird header guards in headers (which IMO might be more common).

[ioeric]

lib/Format/Format.cpp

1472

Okay.

I guess I should've work on the comment skipping FIXME first. I'll work on skipping comments first and then continue with this patch,

[ioeric]

Abandon this patch in favor of http://reviews.llvm.org/D20959

lib/Format/Format.cpp

fixCppIncludeInsertions(StringRef Code, const tooling::Replacements &Replaces,
   if (HeaderInsertions.empty())
     return Replaces;
   tooling::Replacements Result;
   std::set_difference(Replaces.begin(), Replaces.end(),
                       HeaderInsertions.begin(), HeaderInsertions.end(),
                       std::inserter(Result, Result.begin()));
 
   llvm::Regex IncludeRegex(IncludeRegexPattern);
-  llvm::Regex DefineRegex(R"(^[\t\ ]*#[\t\ ]*define[\t\ ]*[^\\]*$)");
+  llvm::Regex IfNDefRegex(R"(^[\t\ ]*#[\t\ ]*ifndef[\t\ ]*([^\\]*)$)");
+  llvm::Regex DefineRegex(R"(^[\t\ ]*#[\t\ ]*define[\t\ ]*([^\\]*)$)");
   SmallVector<StringRef, 4> Matches;
 
   StringRef FileName = Replaces.begin()->getFilePath();
   IncludeCategoryManager Categories(Style, FileName);
 
   // Record the offset of the end of the last include in each category.
   std::map<int, int> CategoryEndOffsets;
   // All possible priorities.
   // Add 0 for main header and INT_MAX for headers that are not in any category.
   std::set<int> Priorities = {0, INT_MAX};
   for (const auto &Category : Style.IncludeCategories)
     Priorities.insert(Category.Priority);
   int FirstIncludeOffset = -1;
   int Offset = 0;
   int AfterHeaderGuard = 0;
   SmallVector<StringRef, 32> Lines;
   Code.split(Lines, '\n');
-  for (auto Line : Lines) {
+  for (auto I = Lines.begin(), B = Lines.begin(), E = Lines.end(); I != E;
+       ++I) {
+    StringRef Line = *I;
     if (IncludeRegex.match(Line, &Matches)) {
       StringRef IncludeName = Matches[2];
       int Category = Categories.getIncludePriority(
           IncludeName, /*CheckMainHeader=*/FirstIncludeOffset < 0);
       CategoryEndOffsets[Category] = Offset + Line.size() + 1;
       if (FirstIncludeOffset < 0)
         FirstIncludeOffset = Offset;
     }
     Offset += Line.size() + 1;
-    // FIXME: make header guard matching stricter, e.g. consider #ifndef.
-    if (AfterHeaderGuard == 0 && DefineRegex.match(Line))
+    if (FirstIncludeOffset < 0 && AfterHeaderGuard == 0 && I != B &&
+        DefineRegex.match(Line, &Matches)) {
+      StringRef DefineName = Matches[1];
+      if (IfNDefRegex.match(*std::prev(I), &Matches) &&
+          DefineName == Matches[1])
         AfterHeaderGuard = Offset;
     }
+  }
 
   // Populate CategoryEndOfssets:
   // - Ensure that CategoryEndOffset[Highest] is always populated.
   // - If CategoryEndOffset[Priority] isn't set, use the next higher value that
   //   is set, up to CategoryEndOffset[Highest].
   // FIXME: skip comment section in the beginning of the code if there is no
   // existing #include and #define.
   auto Highest = Priorities.begin();

unittests/Format/CleanupTest.cpp

TEST_F(CleanUpReplacementsTest, NotConfusedByDefine) {
   std::string Expected = "#include <vector>\n"
                          "void f() {}\n"
                          "#define A \\\n"
                          "  int i;";
   tooling::Replacements Replaces = {createInsertion("#include <vector>")};
   EXPECT_EQ(Expected, formatAndApply(Code, Replaces));
 }
 
+TEST_F(CleanUpReplacementsTest, DontJustInsertAfterRandomDefine) {
+  std::string Code = "#define X 1";
+  std::string Expected = "#include <vector>\n"
+                         "#define X 1";
+  tooling::Replacements Replaces = {createInsertion("#include <vector>")};
+  EXPECT_EQ(Expected, apply(Code, Replaces));
+}
+
+TEST_F(CleanUpReplacementsTest, InsertAfterHeaderGuard) {
+  std::string Code = "#ifndef X_H\n"
+                     "#define X_H\n";
+  std::string Expected = "#ifndef X_H\n"
+                         "#define X_H\n"
+                         "#include <vector>\n";
+  tooling::Replacements Replaces = {createInsertion("#include <vector>")};
+  EXPECT_EQ(Expected, apply(Code, Replaces));
+}
+
+TEST_F(CleanUpReplacementsTest, DontInsertAfterNonMatchedHeaderGuard) {
+  std::string Code = "#ifndef X_H\n"
+                     "#define Y_H";
+  std::string Expected = "#include <vector>\n"
+                         "#ifndef X_H\n"
+                         "#define Y_H";
+  tooling::Replacements Replaces = {createInsertion("#include <vector>")};
+  EXPECT_EQ(Expected, apply(Code, Replaces));
+}
+
 } // end namespace
 } // end namespace format
 } // end namespace clang