[clang-format] skip empty lines and comments in the top of the code when inserting new headers.

[ioeric]

Summary

[clang-format] skip empty lines and comments in the top of the code when inserting new headers.

Pair-programmed with @hokein

[ioeric]

ping

[djasper]

Could we very easily use the FormatTokenLexer for this? I.e. find the first non-comment token an start from there?

[ioeric]

• Use FormatTokenLexer to skip comments.

[djasper]

lib/Format/Format.cpp

1484 ↗

(On Diff #59497)

Thinking about this some more, I think the FormatTokenLexer already does too much. Maybe just use a regular lexer?

const SourceManager& SourceMgr = Env->getSourceManager();
Lexer Lex(Env->getFileID(), SourceMgr.getBuffer(Env->getFileID()), SourceMgr,
          getFormattingLangOpts(Style));
Token Tok;
int MinInsertOffset = Code.size();
while (Lex.LexFromRawLexer(&Tok)) {
  if (Tok.isNot(tok::comment)) {
    MinInsertOffset = SourceMgr.getFileOffset(Tok.getLocation());
    break;
  }
}

[ioeric]

• Use regular Lexer instead of FormatTokenLexer.

[ioeric]

• Switch back to range-base looping for Lines.

[ioeric]

• Nits fixed.

[djasper]

Looks good.

cfe/trunk/lib/Format/Format.cpp

fixCppIncludeInsertions(StringRef Code, const tooling::Replacements &Replaces,
 
   llvm::Regex IncludeRegex(IncludeRegexPattern);
   llvm::Regex DefineRegex(R"(^[\t\ ]*#[\t\ ]*define[\t\ ]*[^\\]*$)");
   SmallVector<StringRef, 4> Matches;
 
   StringRef FileName = Replaces.begin()->getFilePath();
   IncludeCategoryManager Categories(Style, FileName);
 
+  std::unique_ptr<Environment> Env =
+      Environment::CreateVirtualEnvironment(Code, FileName, /*Ranges=*/{});
+  const SourceManager &SourceMgr = Env->getSourceManager();
+  Lexer Lex(Env->getFileID(), SourceMgr.getBuffer(Env->getFileID()), SourceMgr,
+            getFormattingLangOpts(Style));
+  Token Tok;
+  // All new headers should be inserted after this offset.
+  int MinInsertOffset = Code.size();
+  while (!Lex.LexFromRawLexer(Tok)) {
+    if (Tok.isNot(tok::comment)) {
+      MinInsertOffset = SourceMgr.getFileOffset(Tok.getLocation());
+      break;
+    }
+  }
   // 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;
+  bool HeaderGuardFound = false;
+  StringRef TrimmedCode = Code.drop_front(MinInsertOffset);
   SmallVector<StringRef, 32> Lines;
-  Code.split(Lines, '\n');
+  TrimmedCode.split(Lines, '\n');
+  int Offset = MinInsertOffset;
   for (auto Line : Lines) {
     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))
-      AfterHeaderGuard = Offset;
+    if (!HeaderGuardFound && DefineRegex.match(Line)) {
+      HeaderGuardFound = true;
+      MinInsertOffset = 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();
   if (CategoryEndOffsets.find(*Highest) == CategoryEndOffsets.end()) {
     if (FirstIncludeOffset >= 0)
       CategoryEndOffsets[*Highest] = FirstIncludeOffset;
     else
-      CategoryEndOffsets[*Highest] = AfterHeaderGuard;
+      CategoryEndOffsets[*Highest] = MinInsertOffset;
   }
   // By this point, CategoryEndOffset[Highest] is always set appropriately:
   //  - to an appropriate location before/after existing #includes, or
   //  - to right after the header guard, or
   //  - to the beginning of the file.
   for (auto I = ++Priorities.begin(), E = Priorities.end(); I != E; ++I)
     if (CategoryEndOffsets.find(*I) == CategoryEndOffsets.end())
       CategoryEndOffsets[*I] = CategoryEndOffsets[*std::prev(I)];

cfe/trunk/unittests/Format/CleanupTest.cpp

TEST_F(CleanUpReplacementsTest, InsertMultipleIncludesGoogleStyle) {
   tooling::Replacements Replaces = {createInsertion("#include <list>"),
                                     createInsertion("#include \"x/x.h\"")};
   Style = format::getGoogleStyle(format::FormatStyle::LanguageKind::LK_Cpp);
   EXPECT_EQ(Expected, apply(Code, Replaces));
 }
 
 TEST_F(CleanUpReplacementsTest, InsertMultipleNewHeadersAndSortLLVM) {
   std::string Code = "\nint x;";
-  std::string Expected = "#include \"fix.h\"\n"
+  std::string Expected = "\n#include \"fix.h\"\n"
                          "#include \"a.h\"\n"
                          "#include \"b.h\"\n"
                          "#include \"c.h\"\n"
                          "#include <list>\n"
                          "#include <vector>\n"
-                         "\nint x;";
+                         "int x;";
   tooling::Replacements Replaces = {createInsertion("#include \"a.h\""),
                                     createInsertion("#include \"c.h\""),
                                     createInsertion("#include \"b.h\""),
                                     createInsertion("#include <vector>"),
                                     createInsertion("#include <list>"),
                                     createInsertion("#include \"fix.h\"")};
   EXPECT_EQ(Expected, formatAndApply(Code, Replaces));
 }
 
 TEST_F(CleanUpReplacementsTest, InsertMultipleNewHeadersAndSortGoogle) {
   std::string Code = "\nint x;";
-  std::string Expected = "#include \"fix.h\"\n"
+  std::string Expected = "\n#include \"fix.h\"\n"
                          "#include <list>\n"
                          "#include <vector>\n"
                          "#include \"a.h\"\n"
                          "#include \"b.h\"\n"
                          "#include \"c.h\"\n"
-                         "\nint x;";
+                         "int x;";
   tooling::Replacements Replaces = {createInsertion("#include \"a.h\""),
                                     createInsertion("#include \"c.h\""),
                                     createInsertion("#include \"b.h\""),
                                     createInsertion("#include <vector>"),
                                     createInsertion("#include <list>"),
                                     createInsertion("#include \"fix.h\"")};
   Style = format::getGoogleStyle(format::FormatStyle::LanguageKind::LK_Cpp);
   EXPECT_EQ(Expected, formatAndApply(Code, Replaces));
 }
 
 TEST_F(CleanUpReplacementsTest, FormatCorrectLineWhenHeadersAreInserted) {
   std::string Code = "\n"
+                     "int x;\n"
                      "int    a;\n"
                      "int    a;\n"
                      "int    a;";
 
-  std::string Expected = "#include \"x.h\"\n"
+  std::string Expected = "\n#include \"x.h\"\n"
                          "#include \"y.h\"\n"
                          "#include \"clang/x/x.h\"\n"
                          "#include <list>\n"
                          "#include <vector>\n"
-                         "\n"
+                         "int x;\n"
                          "int    a;\n"
                          "int b;\n"
                          "int    a;";
   tooling::Replacements Replaces = {
-      createReplacement(getOffset(Code, 3, 8), 1, "b"),
+      createReplacement(getOffset(Code, 4, 8), 1, "b"),
       createInsertion("#include <vector>"),
       createInsertion("#include <list>"),
       createInsertion("#include \"clang/x/x.h\""),
       createInsertion("#include \"y.h\""),
       createInsertion("#include \"x.h\"")};
   EXPECT_EQ(Expected, formatAndApply(Code, Replaces));
 }
 
 TEST_F(CleanUpReplacementsTest, NotConfusedByDefine) {
   std::string Code = "void f() {}\n"
                      "#define A \\\n"
                      "  int i;";
   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, SkippedTopComment) {
+  std::string Code = "// comment\n"
+                     "\n"
+                     "   // comment\n";
+  std::string Expected = "// comment\n"
+                         "\n"
+                         "   // comment\n"
+                         "#include <vector>\n";
+  tooling::Replacements Replaces = {createInsertion("#include <vector>")};
+  EXPECT_EQ(Expected, apply(Code, Replaces));
+}
+
+TEST_F(CleanUpReplacementsTest, SkippedMixedComments) {
+  std::string Code = "// comment\n"
+                     "// comment \\\n"
+                     " comment continued\n"
+                     "/*\n"
+                     "* comment\n"
+                     "*/\n";
+  std::string Expected = "// comment\n"
+                         "// comment \\\n"
+                         " comment continued\n"
+                         "/*\n"
+                         "* comment\n"
+                         "*/\n"
+                         "#include <vector>\n";
+  tooling::Replacements Replaces = {createInsertion("#include <vector>")};
+  EXPECT_EQ(Expected, apply(Code, Replaces));
+}
+
+TEST_F(CleanUpReplacementsTest, MultipleBlockCommentsInOneLine) {
+  std::string Code = "/*\n"
+                     "* comment\n"
+                     "*/ /* comment\n"
+                     "*/\n"
+                     "\n\n"
+                     "/* c1 */ /*c2 */\n";
+  std::string Expected = "/*\n"
+                         "* comment\n"
+                         "*/ /* comment\n"
+                         "*/\n"
+                         "\n\n"
+                         "/* c1 */ /*c2 */\n"
+                         "#include <vector>\n";
+  tooling::Replacements Replaces = {createInsertion("#include <vector>")};
+  EXPECT_EQ(Expected, apply(Code, Replaces));
+}
+
+TEST_F(CleanUpReplacementsTest, CodeAfterComments) {
+  std::string Code = "/*\n"
+                     "* comment\n"
+                     "*/ /* comment\n"
+                     "*/\n"
+                     "\n\n"
+                     "/* c1 */ /*c2 */\n"
+                     "\n"
+                     "int x;\n";
+  std::string Expected = "/*\n"
+                         "* comment\n"
+                         "*/ /* comment\n"
+                         "*/\n"
+                         "\n\n"
+                         "/* c1 */ /*c2 */\n"
+                         "\n"
+                         "#include <vector>\n"
+                         "int x;\n";
+  tooling::Replacements Replaces = {createInsertion("#include <vector>")};
+  EXPECT_EQ(Expected, apply(Code, Replaces));
+}
+
 } // end namespace
 } // end namespace format
 } // end namespace clang