[PATCH] D11240: Add basic #include sorting functionality to clang-format

[Manuel Klimek via cfe-commits]

klimek added a comment.

First round of comments; some things are still a bit confusing, so I hope another round will help to weed them out.


================
Comment at: include/clang/Tooling/Core/Replacement.h:223-224
@@ -222,1 +222,4 @@
 
+/// \brief Merges to sets of replacements with the second set referring to the
+/// code after applying the first set.
+Replacements mergeReplacements(const Replacements &First,
----------------
s/to/two/


================
Comment at: lib/Format/Format.cpp:1614-1622
@@ +1613,11 @@
+
+  bool OutOfOrder = false;
+  for (unsigned i = 1, e = Indices.size(); i != e; ++i) {
+    if (Indices[i] != i) {
+      OutOfOrder = true;
+      break;
+    }
+  }
+  if (!OutOfOrder)
+    return;
+
----------------
I think this wants a comment along the lines of:
// We create one large replacement for the block of includes.
// If the order is already correct, we do not want to create that replacement.


================
Comment at: lib/Format/Format.cpp:1646
@@ +1645,3 @@
+  unsigned SearchFrom = 0;
+  llvm::Regex IncludeRegex(R"(^\ *#\ *include[^"<]*["<]([^">]*)([">]))");
+  SmallVector<StringRef, 4> Matches;
----------------
Do we want to use \s instead of "\ " so this also works for tab-using codebases?

================
Comment at: lib/Format/Format.cpp:1653
@@ +1652,3 @@
+        Code.substr(Prev, (Pos != StringRef::npos ? Pos : Code.size()) - Prev);
+    if (!Line.endswith("\\")) {
+      if (IncludeRegex.match(Line, &Matches)) {
----------------
I'd have expected early exit, but I can see it'd introduce a one-line duplication. Not sure, just wanted to bring it up.

================
Comment at: lib/Format/Format.cpp:1662-1665
@@ +1661,6 @@
+        IncludesInBlock.push_back({Matches[1], Line, Prev, Matches[2] == ">"});
+      } else {
+        sortIncludes(Style, IncludesInBlock, Ranges, FileName, Replaces);
+        IncludesInBlock.clear();
+      }
+      Prev = Pos + 1;
----------------
Nit: I think this would be a bit more readable if we only call sortInlucdes if !IncludesInBlock.empty() and add that as condition to the else.

================
Comment at: lib/Tooling/Core/Replacement.cpp:310-313
@@ +309,6 @@
+       FirstI != First.end() || SecondI != Second.end();) {
+    // 'MergeSecond' is true, if an element from 'Second' needs to be merged
+    // next, and false if an element from 'First' shoud be merged. As the input
+    // replacements are non-overlapping, we always need to merge an element
+    // from 'Second' into an element from 'First' or vice versa.
+    bool MergeSecond = SecondI == Second.end() ||
----------------
Nit: remove the first ','.
s/shoud/should/
Also: 'non-overlapping in each set'? (or is that incorrect? the function comment in the header doesn't currently tell, I think we might want to change that)

================
Comment at: lib/Tooling/Core/Replacement.cpp:314
@@ +313,3 @@
+    // from 'Second' into an element from 'First' or vice versa.
+    bool MergeSecond = SecondI == Second.end() ||
+                       FirstI->getOffset() < SecondI->getOffset() + Delta;
----------------
Would be good to learn up here why SecondI == Second.end() implies "an element from 'Second' needs to be merged".
Ok, it looks like this implies that the other one is the one that is the basis for merging. Perhaps MergeIntoFirst?

================
Comment at: lib/Tooling/Core/Replacement.cpp:332-336
@@ +331,7 @@
+    // subsequent, non-overlapping replacements.
+    int DeltaFirst = MergeSecond ? MergedText.size() - MergedLength : 0;
+    int DeltaSecond = MergeSecond ? 0 : MergedText.size() - MergedLength;
+    while ((MergeSecond && SecondI != Second.end()) ||
+           (!MergeSecond && FirstI != First.end())) {
+      if (MergeSecond) {
+        // If we are merging an element from 'Second', the length of the
----------------
This structure looks like we'd want 2 functions. I assume you had reasons not to pull out two local functions? What are they? Too many parameters? (I'd probably pull out a class to be able to put this into smaller methods, but I'm probably weird).

================
Comment at: lib/Tooling/Core/Replacement.cpp:341
@@ +340,3 @@
+        unsigned MergedEnd = MergedOffset + MergedText.size();
+        unsigned Offset = SecondI->getOffset() + DeltaSecond + Delta;
+        if (Offset > MergedEnd)
----------------
If SecondI == Second.end(), isn't this an undefined access?

================
Comment at: lib/Tooling/Core/Replacement.cpp:354
@@ +353,3 @@
+        StringRef Tail = MergedRef.substr(End - MergedOffset);
+        MergedText = (Twine(Head) + SecondI->getReplacementText() + Tail).str();
+        DeltaSecond += SecondI->getReplacementText().size() - Length;
----------------
I'm surprised the explicit Twine() is needed here...


http://reviews.llvm.org/D11240