[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: lib/Format/Format.cpp:1411
@@ +1410,3 @@
+int getIncludePriority(const FormatStyle &Style,
+                       SmallVector<llvm::Regex, 4> &CategoryRegexs,
+                       StringRef IncludeName) {
----------------
But you should pass it as an ArrayRef or a SmallVectorImpl.

================
Comment at: lib/Format/Format.cpp:1413
@@ +1412,3 @@
+                       StringRef IncludeName) {
+  for (unsigned i = 0, e = CategoryRegexs.size(); i != e; ++i) {
+    if (CategoryRegexs[i].match(IncludeName)) {
----------------
nit: no braces

================
Comment at: lib/Format/Format.cpp:1418
@@ +1417,3 @@
+  }
+  return -1;
+}
----------------
>From the docs in Format.h: "If none of the regular expressions match, INT_MAX is assigned". If that is true for an existing #include as well as the inserted one, I don't see a reason, why those should belong to the same block. So, I think you should return INT_MAX here and remove all the special-casing below.

================
Comment at: lib/Format/Format.cpp:1431
@@ +1430,3 @@
+
+  llvm::Regex IncludeRegex(
+      R"(^[\t\ ]*#[\t\ ]*include[\t\ ]*(["<][^">]*[">]))");
----------------
We already have an include regex in this file, I don't think we should have two. Can you pull out the string into a constant?

================
Comment at: lib/Format/Format.cpp:1435
@@ +1434,3 @@
+  SmallVector<StringRef, 4> Matches;
+  // Create pre-compiled regular expressions for the #include categories.
+  SmallVector<llvm::Regex, 4> CategoryRegexs;
----------------
There is a lot of duplicated code between this and sortCppIncludes. Pull common code out into functions.

================
Comment at: lib/Format/Format.cpp:1454
@@ +1453,3 @@
+  // extra category (priority 0) for main include.
+  std::vector<int> CategoryEndOffsets(MaxPriority + 1, -1);
+  bool MainIncludeFound = false;
----------------
There is no guarantee that people assign categories sequentially, I can easily see people assigning category "1000" or "10000" to be able to add more categories later. I think this should be a map and I think you then also don't really need the vector. Also note that categories can be negative, which I think would break this.

================
Comment at: lib/Format/Format.cpp:1458
@@ +1457,3 @@
+  int Offset = 0;
+  int AfterFirstDefine = 0;
+  SmallVector<StringRef, 32> Lines;
----------------
I am guessing that AfterFirstDefine is used so that we insert #includes after header guards. I think we should name it AfterHeaderGuard than and either make the behavior more strict or at least add a FIXME for it (only in header files, only if the #define is after an #ifndef, ...)

================
Comment at: lib/Format/Format.cpp:1482
@@ +1481,3 @@
+      } else {
+        CategoryEndOffsets[Category] = Offset + Line.size() + 1;
+      }
----------------
I think if you assign the EndOffset of all lower categories that aren't set yet, that will simplify the logic below.

================
Comment at: lib/Format/Format.cpp:1509
@@ +1508,3 @@
+    // Append the new #include after the corresponding category if the category
+    // exists. Otherwise, append after the last category that has higher
+    // priority than the current category. If there is no category, insert after
----------------
What happens if there is no category with higher priority, but there is a category with lower priority. Shouldn't you then insert the #include right before that?

================
Comment at: lib/Format/Format.cpp:1548
@@ +1547,3 @@
+                                          const FormatStyle &Style) {
+  // FIXME: support other languages.
+  if (Style.Language != FormatStyle::LanguageKind::LK_Cpp)
----------------
I think you can remove this FIXME.. Seems obvious.

================
Comment at: lib/Format/Format.cpp:1551
@@ +1550,3 @@
+    return Replaces;
+  tooling::Replacements HeaderInsertionReplaces;
+  tooling::Replacements NewReplaces;
----------------
Why do you split out all the header insertion replacements here instead of just ignoring the ones that aren't header insertions in fixCppIncludeInsertions?

================
Comment at: lib/Format/Format.cpp:1554
@@ +1553,3 @@
+  for (const auto &R : Replaces) {
+    if (R.getOffset() == -1U) {
+      HeaderInsertionReplaces.insert(R);
----------------
nit: No braces.


http://reviews.llvm.org/D20734