[PATCH] D15639: [clang-format] Ensure Sort include is stable with negative Priority

[Jean-Philippe Dufraigne via cfe-commits]

jeanphilippeD created this revision.
jeanphilippeD added a reviewer: djasper.
jeanphilippeD added a subscriber: cfe-commits.
Herald added a subscriber: klimek.

The new sort include with negative priority was not stable when running it 
again over the updated includes.

Extend the test NegativePriorities to have an extra header. Test pas still 
passing
Add a new assertion to test that the sorting is stable. This new assertion 
failed.

From this:
#include "important_os_header.h"
#include "c_main_header.h"
#include "a_other_header.h"

Before fix:
#include "important_os_header.h"
#include "a_other_header.h"
#include "c_main_header.h"

After fix:
#include "important_os_header.h"
#include "c_main_header.h"
#include "a_other_header.h"

http://reviews.llvm.org/D15639

Files:
  lib/Format/Format.cpp
  unittests/Format/SortIncludesTest.cpp

Index: unittests/Format/SortIncludesTest.cpp
===================================================================
--- unittests/Format/SortIncludesTest.cpp
+++ unittests/Format/SortIncludesTest.cpp
@@ -188,9 +188,19 @@
 TEST_F(SortIncludesTest, NegativePriorities) {
   Style.IncludeCategories = {{".*important_os_header.*", -1}, {".*", 1}};
   EXPECT_EQ("#include \"important_os_header.h\"\n"
-            "#include \"a.h\"\n",
-            sort("#include \"a.h\"\n"
+            "#include \"c_main_header.h\"\n"
+            "#include \"a_other_header.h\"\n",
+            sort("#include \"c_main_header.h\"\n"
+                 "#include \"a_other_header.h\"\n"
                  "#include \"important_os_header.h\"\n"));
+
+  // check stable when re-run
+  EXPECT_EQ("#include \"important_os_header.h\"\n"
+            "#include \"c_main_header.h\"\n"
+            "#include \"a_other_header.h\"\n",
+            sort("#include \"important_os_header.h\"\n"
+                 "#include \"c_main_header.h\"\n"
+                 "#include \"a_other_header.h\"\n"));
 }
 
 TEST_F(SortIncludesTest, CalculatesCorrectCursorPosition) {
Index: lib/Format/Format.cpp
===================================================================
--- lib/Format/Format.cpp
+++ lib/Format/Format.cpp
@@ -1807,19 +1807,21 @@
     if (!FormattingOff && !Line.endswith("\\")) {
       if (IncludeRegex.match(Line, &Matches)) {
         StringRef IncludeName = Matches[2];
-        int Category;
-        if (LookForMainHeader && !IncludeName.startswith("<")) {
-          Category = 0;
-        } else {
-          Category = INT_MAX;
-          for (unsigned i = 0, e = CategoryRegexs.size(); i != e; ++i) {
+        int Category = INT_MAX;
+        for (unsigned i = 0, e = CategoryRegexs.size(); i != e; ++i) {
             if (CategoryRegexs[i].match(IncludeName)) {
-              Category = Style.IncludeCategories[i].Priority;
-              break;
+                Category = Style.IncludeCategories[i].Priority;
+                break;
             }
-          }
         }
-        LookForMainHeader = false;
+
+        if (Category >= 0 ) {
+            if (LookForMainHeader && !IncludeName.startswith("<")) {
+                Category = 0;
+            }
+            LookForMainHeader = false;
+        }
+
         IncludesInBlock.push_back({IncludeName, Line, Prev, Category});
       } else if (!IncludesInBlock.empty()) {
         sortIncludes(Style, IncludesInBlock, Ranges, FileName, Replaces,



Index: unittests/Format/SortIncludesTest.cpp
===================================================================
--- unittests/Format/SortIncludesTest.cpp
+++ unittests/Format/SortIncludesTest.cpp
@@ -188,9 +188,19 @@
 TEST_F(SortIncludesTest, NegativePriorities) {
   Style.IncludeCategories = {{".*important_os_header.*", -1}, {".*", 1}};
   EXPECT_EQ("#include \"important_os_header.h\"\n"
-            "#include \"a.h\"\n",
-            sort("#include \"a.h\"\n"
+            "#include \"c_main_header.h\"\n"
+            "#include \"a_other_header.h\"\n",
+            sort("#include \"c_main_header.h\"\n"
+                 "#include \"a_other_header.h\"\n"
                  "#include \"important_os_header.h\"\n"));
+
+  // check stable when re-run
+  EXPECT_EQ("#include \"important_os_header.h\"\n"
+            "#include \"c_main_header.h\"\n"
+            "#include \"a_other_header.h\"\n",
+            sort("#include \"important_os_header.h\"\n"
+                 "#include \"c_main_header.h\"\n"
+                 "#include \"a_other_header.h\"\n"));
 }
 
 TEST_F(SortIncludesTest, CalculatesCorrectCursorPosition) {
Index: lib/Format/Format.cpp
===================================================================
--- lib/Format/Format.cpp
+++ lib/Format/Format.cpp
@@ -1807,19 +1807,21 @@
     if (!FormattingOff && !Line.endswith("\\")) {
       if (IncludeRegex.match(Line, &Matches)) {
         StringRef IncludeName = Matches[2];
-        int Category;
-        if (LookForMainHeader && !IncludeName.startswith("<")) {
-          Category = 0;
-        } else {
-          Category = INT_MAX;
-          for (unsigned i = 0, e = CategoryRegexs.size(); i != e; ++i) {
+        int Category = INT_MAX;
+        for (unsigned i = 0, e = CategoryRegexs.size(); i != e; ++i) {
             if (CategoryRegexs[i].match(IncludeName)) {
-              Category = Style.IncludeCategories[i].Priority;
-              break;
+                Category = Style.IncludeCategories[i].Priority;
+                break;
             }
-          }
         }
-        LookForMainHeader = false;
+
+        if (Category >= 0 ) {
+            if (LookForMainHeader && !IncludeName.startswith("<")) {
+                Category = 0;
+            }
+            LookForMainHeader = false;
+        }
+
         IncludesInBlock.push_back({IncludeName, Line, Prev, Category});
       } else if (!IncludesInBlock.empty()) {
         sortIncludes(Style, IncludesInBlock, Ranges, FileName, Replaces,

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits