[clang-format] do not add existing includes.

[ioeric]

Summary

do not add existing includes.

[djasper]

I am happy to get this in for now, but I actually think the right solution might be to let clang-format de-duplicate #includes in general. What do you think?

[ioeric]

It should be quite straight-forward to de-duplicate #includes in the same block in sortIncludes. I'm happy to add this into sortIncludes, but I'm not sure if this is a behavior users would expect from clang-format since it deletes code?

And maybe duplicated headers are not that common in the first place? I would rather rely on users (like #include_fixer) to make sure there is no duplicated #include.

[djasper]

Well, it is easy to not see duplicated #includes when they aren't properly sorted. When clang-format then goes in and sorts them, they end up next to each other. And I think no user really wants that.

[ioeric]

Agreed. So, do we still want to skip existing #includes here or simply rely on sortIncludes to de-duplicate?

[djasper]

Lets get this in for now. We can always remove it if we think it is no longer useful.

unittests/Format/CleanupTest.cpp

692 ↗

(On Diff #60669)

Please also add a test with quoted #includes as well as combined forms (inserting <vector> when there already is "vector" and vice versa).

[ioeric]

• Addressed reviewer's comments: test case added.

[djasper]

unittests/Format/CleanupTest.cpp

693 ↗

(On Diff #60675)

Please add a line break after each "\n", i.e.:

... = "#include \"a.h\"\n"
      "#include <vector>";

701 ↗

(On Diff #60675)

Is this really what we want?

[ioeric]

• addressed reviewer's comments.

unittests/Format/CleanupTest.cpp

701 ↗

(On Diff #60675)

I'm not sure... this might not be the best behavior. FIXME added.

[djasper]

Looks good :-)

cfe/trunk/lib/Format/Format.cpp

unsigned getOffsetAfterHeaderGuardsAndComments(StringRef FileName,
   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)
     return Replaces;
 
   tooling::Replacements HeaderInsertions;
fixCppIncludeInsertions(StringRef Code, const tooling::Replacements &Replaces,
   // All new headers should be inserted after this offset.
   unsigned MinInsertOffset =
       getOffsetAfterHeaderGuardsAndComments(FileName, Code, Style);
   StringRef TrimmedCode = Code.drop_front(MinInsertOffset);
   SmallVector<StringRef, 32> Lines;
   TrimmedCode.split(Lines, '\n');
   unsigned Offset = MinInsertOffset;
   unsigned NextLineOffset;
+  std::set<StringRef> ExistingIncludes;
   for (auto Line : Lines) {
     NextLineOffset = std::min(Code.size(), Offset + Line.size() + 1);
     if (IncludeRegex.match(Line, &Matches)) {
       StringRef IncludeName = Matches[2];
+      ExistingIncludes.insert(IncludeName);
       int Category = Categories.getIncludePriority(
           IncludeName, /*CheckMainHeader=*/FirstIncludeOffset < 0);
       CategoryEndOffsets[Category] = NextLineOffset;
       if (FirstIncludeOffset < 0)
         FirstIncludeOffset = Offset;
     }
     Offset = NextLineOffset;
   }
fixCppIncludeInsertions(StringRef Code, const tooling::Replacements &Replaces,
 
   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];
+    if (ExistingIncludes.find(IncludeName) != ExistingIncludes.end()) {
+      DEBUG(llvm::dbgs() << "Skip adding existing include : " << IncludeName
+                         << "\n");
+      continue;
+    }
     int Category =
         Categories.getIncludePriority(IncludeName, /*CheckMainHeader=*/true);
     Offset = CategoryEndOffsets[Category];
     std::string NewInclude = !IncludeDirective.endswith("\n")
                                  ? (IncludeDirective + "\n").str()
                                  : IncludeDirective.str();
     Result.insert(tooling::Replacement(FileName, Offset, 0, NewInclude));
   }

cfe/trunk/unittests/Format/CleanupTest.cpp

 // 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>")};
   EXPECT_EQ(Expected, apply(Code, Replaces));
 }
 
+TEST_F(CleanUpReplacementsTest, SkipExistingHeaders) {
+  std::string Code = "#include \"a.h\"\n"
+                     "#include <vector>\n";
+  std::string Expected = "#include \"a.h\"\n"
+                         "#include <vector>\n";
+  tooling::Replacements Replaces = {createInsertion("#include <vector>"),
+                                    createInsertion("#include \"a.h\"")};
+  EXPECT_EQ(Expected, apply(Code, Replaces));
+}
+
+TEST_F(CleanUpReplacementsTest, AddIncludesWithDifferentForms) {
+  std::string Code = "#include \"a.h\"\n"
+                     "#include <vector>\n";
+  // FIXME: this might not be the best behavior.
+  std::string Expected = "#include \"a.h\"\n"
+                         "#include \"vector\"\n"
+                         "#include <vector>\n"
+                         "#include <a.h>\n";
+  tooling::Replacements Replaces = {createInsertion("#include \"vector\""),
+                                    createInsertion("#include <a.h>")};
+  EXPECT_EQ(Expected, apply(Code, Replaces));
+}
+
 } // end namespace
 } // end namespace format
 } // end namespace clang