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

[Daniel Jasper via cfe-commits]

djasper marked 8 inline comments as done.

================
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)) {
----------------
klimek wrote:
> 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.
It's actually three lines I think. Also, I think we'll significantly extend the logic here shortly (e.g. to properly support Google and LLVM styles), so I don't think it is worth thinking about too much.

================
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;
----------------
klimek wrote:
> 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.
Done and removed the check inside sortIncludes.

================
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
----------------
klimek wrote:
> 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).
Maybe a class would be the way to go here. I'll try in a subsequent patch (don't think it should be a priority at this point).

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

================
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;
----------------
klimek wrote:
> I'm surprised the explicit Twine() is needed here...
How else would this know to use a Twine for efficient concatenation?


http://reviews.llvm.org/D11240