[clang-format] append newline after code when inserting new headers at the end of the code which does not end with newline.

[ioeric]

Summary

append newline after code when inserting new headers at the end of the
code which does not end with newline.

[djasper]

Does this still work with the new Replacements?

lib/Format/Format.cpp

1572

"does not end"

1574

"whether"

[djasper]

Ping?

[ioeric]

• Merged with origin/master.
• Update newline insertion according to the new Replacements implementation.

[ioeric]

Sorry for the delay....

I've updated the patch to work with the new tooling::Replacements.

[djasper]

Looks good.

lib/Format/Format.cpp

unsigned getOffsetAfterHeaderGuardsAndComments(StringRef FileName,
   if (checkAndConsumeDirectiveWithName(Lex, "ifndef", Tok)) {
     skipComments(Lex, Tok);
     if (checkAndConsumeDirectiveWithName(Lex, "define", Tok))
       return SourceMgr.getFileOffset(Tok.getLocation());
   }
   return AfterComments;
 }
 
-// FIXME: we also need to insert a '\n' at the end of the code if we have an
-// insertion with offset Code.size(), and there is no '\n' at the end of the
-// code.
 // FIXME: do not insert headers into conditional #include blocks, e.g. #includes
 // surrounded by compile condition "#if...".
 // FIXME: do not insert existing headers.
 // FIXME: insert empty lines between newly created blocks.
 tooling::Replacements
 fixCppIncludeInsertions(StringRef Code, const tooling::Replacements &Replaces,
                         const FormatStyle &Style) {
   if (Style.Language != FormatStyle::LanguageKind::LK_Cpp)
fixCppIncludeInsertions(StringRef Code, const tooling::Replacements &Replaces,
   // 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)];
 
+  bool NeedNewLineAtEnd = !Code.empty() && Code.back() != '\n';
   for (const auto &R : HeaderInsertions) {
     auto IncludeDirective = R.getReplacementText();
     bool Matched = IncludeRegex.match(IncludeDirective, &Matches);
     assert(Matched && "Header insertion replacement must have replacement text "
                       "'#include ...'");
     (void)Matched;
     auto IncludeName = Matches[2];
     int Category =
         Categories.getIncludePriority(IncludeName, /*CheckMainHeader=*/true);
     Offset = CategoryEndOffsets[Category];
+    // When inserting headers at end of the code, also insert a '\n' at the end
+    // of the code if it does not ends with '\n'.
+    // The way of inserting '\n' is a bit of hack since we have no control over
+    // wether header insertions with the same offset (i.e. `Code.size()`) are
+    // inserted after the new line insertion ('\n' would actually be inserted
+    // after other insertions with the same offset). To walk around this,
+    // instead of inserting a '\n' at the end, we replace the last character of
+    // the code with the character itself plus a '\n' character.
+    if (NeedNewLineAtEnd && Offset == Code.size()) {
+      auto End = (Code.substr(Code.size()-1, Code.size()) + "\n").str();
+      Result.insert(tooling::Replacement(FileName, Offset-1, 1, End));
+      NeedNewLineAtEnd = false;
+    }
     std::string NewInclude = !IncludeDirective.endswith("\n")
                                  ? (IncludeDirective + "\n").str()
                                  : IncludeDirective.str();
     Result.insert(tooling::Replacement(FileName, Offset, 0, NewInclude));
   }
   return Result;
 }
 

unittests/Format/CleanupTest.cpp

 
 TEST_F(CleanUpReplacementsTest, EmptyCode) {
   std::string Code = "";
   std::string Expected = "#include <vector>\n";
   tooling::Replacements Replaces = {createInsertion("#include <vector>")};
   EXPECT_EQ(Expected, apply(Code, Replaces));
 }
 
-// FIXME: although this case does not crash, the insertion is wrong. A '\n'
-// should be inserted between the two #includes.
 TEST_F(CleanUpReplacementsTest, NoNewLineAtTheEndOfCode) {
-  std::string Code = "#include <map>";
-  std::string Expected = "#include <map>#include <vector>\n";
-  tooling::Replacements Replaces = {createInsertion("#include <vector>")};
+  std::string Code = "#include \"fix.h\"";
+  std::string Expected =
+      "#include \"fix.h\"\n#include <vector>\n#include \"x.h\"\n";
+  tooling::Replacements Replaces = {createInsertion("#include <vector>"),
+                                    createInsertion("#include \"x.h\"")};
   EXPECT_EQ(Expected, apply(Code, Replaces));
 }
 
 } // end namespace
 } // end namespace format
 } // end namespace clang