[PATCH] D20734: [clang-format] insert new #includes into correct blocks when cleaning up Replacement with cleanupAroundReplacements().

[Daniel Jasper via cfe-commits]

djasper added inline comments.

================
Comment at: include/clang/Format/Format.h:778
@@ -776,1 +777,3 @@
+/// replacement corresponding to the header insertion has offset UINT_MAX (i.e.
+/// -1U).
 tooling::Replacements
----------------
Why don't we just stick with UINT_MAX everywhere?

================
Comment at: lib/Format/Format.cpp:1457
@@ +1456,3 @@
+  IncludeCategoryManager Categories(Style, FileName);
+  int MaxPriority = 0;
+  for (const auto &Category : Style.IncludeCategories)
----------------
Unused

================
Comment at: lib/Format/Format.cpp:1466
@@ +1465,3 @@
+  // Add 0 for main header and INT_MAX for headers that are not in any category.
+  std::vector<int> Priorities = {0, INT_MAX};
+  for (const auto &Category : Style.IncludeCategories)
----------------
Why not just a set?

================
Comment at: lib/Format/Format.cpp:1488
@@ +1487,3 @@
+    Offset += Line.size() + 1;
+    // FIXME: make header guard matching more strict, e.g. consider #ifndef.
+    if (AfterHeaderGuard == 0 && DefineRegex.match(Line))
----------------
nit: s/more strict/stricter/

================
Comment at: lib/Format/Format.cpp:1494
@@ +1493,3 @@
+  // Set EndOffset of each category that is not set yet to be the end of the
+  // last category with higher priority. If there is no category with higher
+  // priority, then we look for the next block with lower category; if such
----------------
Are you sure that this is what's implemented? The way I see it is:
- Ensure that EndOffset[Highest] is always populated.
- If EndOffset[Priority] isn't set, use the next higher value that is set, up to EndOffset[Highest].

================
Comment at: lib/Format/Format.cpp:1513
@@ +1512,3 @@
+    } else {
+      // Insert a new line after header guard.
+      // FIXME: since the new line and the end offset are both set to
----------------
I'd leave all of this logic out. This only affects headers with no current #includes, which is rare to begin with and on top of that, this is more to do with whether or not we want to automatically add new lines between the different blocks. As it currently is, this would do both:

  #define HEADER_GUARD

  #include <a> // inserted

and

  #define HEADER_GUARD
  #include <a> // inserted
  #include <b>

Which might be the right behavior, but I think this is the wrong place to worry about it. In the long run, we should probably make clang-format inserted empty lines around the #include blocks if desired.

================
Comment at: lib/Format/Format.cpp:1525
@@ +1524,3 @@
+  }
+  for (int I = 1, E = Priorities.size(); I != E; I++)
+    if (CategoryEndOffsets.find(Priorities[I]) == CategoryEndOffsets.end())
----------------
Add a comment:

  // 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.

================
Comment at: lib/Format/Format.cpp:1531
@@ +1530,3 @@
+    auto IncludeDirective = R.getReplacementText();
+    if (!IncludeRegex.match(IncludeDirective, &Matches)) {
+      llvm::errs() << IncludeDirective
----------------
This should be done together with the check for the offset being -1u. Eventually we are going to have something else that we are going to insert (e.g. using declarations) and we don't want to confuse those. And even more so, if all replacements at offset -1u are not #includes, we don't need to execute most of this function.

================
Comment at: lib/Format/Format.cpp:1538
@@ +1537,3 @@
+    int Category =
+        Categories.getIncludePriority(IncludeName, /*CheckMainHeader=*/false);
+    Offset = CategoryEndOffsets[Category];
----------------
Why is CheckMainHeader false?

================
Comment at: lib/Format/Format.cpp:1541
@@ +1540,3 @@
+    std::string NewInclude =
+        (!IncludeDirective.endswith("\n"))
+            ? (IncludeDirective + "\n").str()
----------------
No parentheses.

================
Comment at: lib/Format/Format.cpp:1550
@@ +1549,3 @@
+// Insert #include directives into the correct blocks.
+tooling::Replacements fixHeaderInsertions(StringRef Code,
+                                          const tooling::Replacements &Replaces,
----------------
Hm.. Not happy with the naming. There is no real difference between fixHeaderInsertions and fixCppIncludeInsertions. You could argue that the "Cpp" part is the difference, but the actual difference is that one iterates over an entire set of replacements whereas the other only modifies header insertions.

I think I'd just merge the two functions, but that probably also ties back to my wish not to separate out the two replacement sets ;-).


http://reviews.llvm.org/D20734