[PATCH] D95017: [clang-format] add case aware include sorting

2021-01-21 Thread Kent Sommer via Phabricator via cfe-commits 
kentsommer added a comment.

I do not have commit access.

Full Name: Kent Sommer
Email: w...@kentsommer.com


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D95017/new/

https://reviews.llvm.org/D95017

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

[PATCH] D95017: [clang-format] add case aware include sorting

2021-01-24 Thread Kent Sommer via Phabricator via cfe-commits 
kentsommer updated this revision to Diff 318886.
kentsommer marked 5 inline comments as done.
kentsommer added a comment.

Addressed comments


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D95017/new/

https://reviews.llvm.org/D95017

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/include/clang/Tooling/Inclusions/IncludeStyle.h
  clang/lib/Format/Format.cpp
  clang/unittests/Format/FormatTest.cpp
  clang/unittests/Format/SortIncludesTest.cpp

Index: clang/unittests/Format/SortIncludesTest.cpp
===
--- clang/unittests/Format/SortIncludesTest.cpp
+++ clang/unittests/Format/SortIncludesTest.cpp
@@ -598,6 +598,49 @@
  "a.cc"));
 }
 
+TEST_F(SortIncludesTest, SupportOptionalAlphabeticalSorting) {
+  EXPECT_FALSE(Style.IncludeSortAlphabetically);
+
+  Style.IncludeSortAlphabetically = true;
+
+  EXPECT_EQ("#include \"A/B.h\"\n"
+"#include \"A/b.h\"\n"
+"#include \"a/b.h\"\n"
+"#include \"B/A.h\"\n"
+"#include \"B/a.h\"\n",
+sort("#include \"B/a.h\"\n"
+ "#include \"B/A.h\"\n"
+ "#include \"A/B.h\"\n"
+ "#include \"a/b.h\"\n"
+ "#include \"A/b.h\"\n",
+ "a.h"));
+
+  Style.IncludeBlocks = clang::tooling::IncludeStyle::IBS_Regroup;
+  Style.IncludeCategories = {
+  {"^\"", 1, 0, false}, {"^<.*\\.h>$", 2, 0, false}, {"^<", 3, 0, false}};
+
+  StringRef UnsortedCode = "#include \"qt.h\"\n"
+   "#include \n"
+   "#include \n"
+   "#include \n"
+   "#include \n"
+   "#include \"vlib.h\"\n"
+   "#include \"Vlib.h\"\n"
+   "#include \"AST.h\"\n";
+
+  EXPECT_EQ("#include \"AST.h\"\n"
+"#include \"qt.h\"\n"
+"#include \"Vlib.h\"\n"
+"#include \"vlib.h\"\n"
+"\n"
+"#include \n"
+"#include \n"
+"\n"
+"#include \n"
+"#include \n",
+sort(UnsortedCode));
+}
+
 TEST_F(SortIncludesTest, SupportCaseInsensitiveMatching) {
   // Setup an regex for main includes so we can cover those as well.
   Style.IncludeIsMainRegex = "([-_](test|unittest))?$";
Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -14395,6 +14395,8 @@
   CHECK_PARSE_BOOL(DeriveLineEnding);
   CHECK_PARSE_BOOL(DerivePointerAlignment);
   CHECK_PARSE_BOOL_FIELD(DerivePointerAlignment, "DerivePointerBinding");
+  CHECK_PARSE_BOOL_FIELD(IncludeStyle.IncludeSortAlphabetically,
+ "IncludeSortAlphabetically");
   CHECK_PARSE_BOOL(DisableFormat);
   CHECK_PARSE_BOOL(IndentCaseLabels);
   CHECK_PARSE_BOOL(IndentCaseBlocks);
Index: clang/lib/Format/Format.cpp
===
--- clang/lib/Format/Format.cpp
+++ clang/lib/Format/Format.cpp
@@ -556,6 +556,8 @@
 IO.mapOptional("IncludeIsMainRegex", Style.IncludeStyle.IncludeIsMainRegex);
 IO.mapOptional("IncludeIsMainSourceRegex",
Style.IncludeStyle.IncludeIsMainSourceRegex);
+IO.mapOptional("IncludeSortAlphabetically",
+   Style.IncludeStyle.IncludeSortAlphabetically);
 IO.mapOptional("IndentCaseLabels", Style.IndentCaseLabels);
 IO.mapOptional("IndentCaseBlocks", Style.IndentCaseBlocks);
 IO.mapOptional("IndentGotoLabels", Style.IndentGotoLabels);
@@ -925,6 +927,7 @@
   {".*", 1, 0, false}};
   LLVMStyle.IncludeStyle.IncludeIsMainRegex = "(Test)?$";
   LLVMStyle.IncludeStyle.IncludeBlocks = tooling::IncludeStyle::IBS_Preserve;
+  LLVMStyle.IncludeStyle.IncludeSortAlphabetically = false;
   LLVMStyle.IndentCaseLabels = false;
   LLVMStyle.IndentCaseBlocks = false;
   LLVMStyle.IndentGotoLabels = true;
@@ -2179,10 +2182,23 @@
   for (unsigned i = 0, e = Includes.size(); i != e; ++i) {
 Indices.push_back(i);
   }
-  llvm::stable_sort(Indices, [&](unsigned LHSI, unsigned RHSI) {
-return std::tie(Includes[LHSI].Priority, Includes[LHSI].Filename) <
-   std::tie(Includes[RHSI].Priority, Includes[RHSI].Filename);
-  });
+
+  if (Style.IncludeStyle.IncludeSortAlphabetically) {
+llvm::stable_sort(Indices, [&](unsigned LHSI, unsigned RHSI) {
+  const auto LHSFilenameLower = Includes[LHSI].Filename.lower();
+  const auto RHSFilenameLower = Includes[RHSI].Filename.lower();
+  return std::tie(Includes[LHSI].Priority, LHSFilenameLower,
+  Includes[LHSI].Filename) <
+ std::tie(Includes[RHSI].Priority, RHSFilenameLower,
+  Includes[RHSI].Filename);
+});
+  } else {
+llvm::stable_sort(Indices, [&](unsigned LHSI, u

[PATCH] D95017: [clang-format] add case aware include sorting

2021-01-24 Thread Kent Sommer via Phabricator via cfe-commits 
kentsommer added inline comments.



Comment at: clang/docs/ClangFormatStyleOptions.rst:2039
 
+**IncludeSortCaseAware** (``bool``)
+  Specify if sorting should be done in a case aware fashion.

MyDeveloperDay wrote:
> curdeius wrote:
> > The name is somewhat awkward IMO (in the sense it doesn't read nicely), but 
> > it's nice to be (alphabetically) closer to other Include-related options in 
> > the documentation.
> > I have no better name though. Just noticing.
> I agree how about `IncludeSortCaseSensitive`
I went with `IncludeSortAlphabetically` for now. To me, this reads better and 
seems to more clearly indicate the effect of turning this on. The downside is 
it no longer captures the fact that case does still play a part in the sort.

What do you both think?



Comment at: clang/docs/ClangFormatStyleOptions.rst:2045
+
+  .. code-block:: c++
+

curdeius wrote:
> I'd write this in the same manner as other options, so sth like "When false: 
> ... When true: ".
> 
> Otherwise, it's necessary to read up to the end of the description "This 
> option is off by default" and conclude that off=false and that was the first 
> snippet that corresponded to false.
Good call, I definitely agree. I've rewritten this to better match the `bool` 
flag instances.



Comment at: clang/lib/Format/Format.cpp:2185
   llvm::stable_sort(Indices, [&](unsigned LHSI, unsigned RHSI) {
+if (Style.IncludeStyle.IncludeSortCaseAware) {
+  const auto LHSFilenameLower = Includes[LHSI].Filename.lower();

curdeius wrote:
> Maybe it would be better for CodeGen to get the if outside of sort? It would 
> mean some code duplication, but maybe it could be mitigated with a lambda.
This is a good point. I moved the `if` outside of the call to `stable_sort`. I 
don't feel there is actually much code duplication caused by this (other than 
the call signature). 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D95017/new/

https://reviews.llvm.org/D95017

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

[PATCH] D95017: [clang-format] add case aware include sorting

2021-01-24 Thread Kent Sommer via Phabricator via cfe-commits 
kentsommer updated this revision to Diff 31.
kentsommer added a comment.

Undo some accidental comment formatting


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D95017/new/

https://reviews.llvm.org/D95017

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/include/clang/Tooling/Inclusions/IncludeStyle.h
  clang/lib/Format/Format.cpp
  clang/unittests/Format/FormatTest.cpp
  clang/unittests/Format/SortIncludesTest.cpp

Index: clang/unittests/Format/SortIncludesTest.cpp
===
--- clang/unittests/Format/SortIncludesTest.cpp
+++ clang/unittests/Format/SortIncludesTest.cpp
@@ -598,6 +598,49 @@
  "a.cc"));
 }
 
+TEST_F(SortIncludesTest, SupportOptionalAlphabeticalSorting) {
+  EXPECT_FALSE(Style.IncludeSortAlphabetically);
+
+  Style.IncludeSortAlphabetically = true;
+
+  EXPECT_EQ("#include \"A/B.h\"\n"
+"#include \"A/b.h\"\n"
+"#include \"a/b.h\"\n"
+"#include \"B/A.h\"\n"
+"#include \"B/a.h\"\n",
+sort("#include \"B/a.h\"\n"
+ "#include \"B/A.h\"\n"
+ "#include \"A/B.h\"\n"
+ "#include \"a/b.h\"\n"
+ "#include \"A/b.h\"\n",
+ "a.h"));
+
+  Style.IncludeBlocks = clang::tooling::IncludeStyle::IBS_Regroup;
+  Style.IncludeCategories = {
+  {"^\"", 1, 0, false}, {"^<.*\\.h>$", 2, 0, false}, {"^<", 3, 0, false}};
+
+  StringRef UnsortedCode = "#include \"qt.h\"\n"
+   "#include \n"
+   "#include \n"
+   "#include \n"
+   "#include \n"
+   "#include \"vlib.h\"\n"
+   "#include \"Vlib.h\"\n"
+   "#include \"AST.h\"\n";
+
+  EXPECT_EQ("#include \"AST.h\"\n"
+"#include \"qt.h\"\n"
+"#include \"Vlib.h\"\n"
+"#include \"vlib.h\"\n"
+"\n"
+"#include \n"
+"#include \n"
+"\n"
+"#include \n"
+"#include \n",
+sort(UnsortedCode));
+}
+
 TEST_F(SortIncludesTest, SupportCaseInsensitiveMatching) {
   // Setup an regex for main includes so we can cover those as well.
   Style.IncludeIsMainRegex = "([-_](test|unittest))?$";
Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -14395,6 +14395,8 @@
   CHECK_PARSE_BOOL(DeriveLineEnding);
   CHECK_PARSE_BOOL(DerivePointerAlignment);
   CHECK_PARSE_BOOL_FIELD(DerivePointerAlignment, "DerivePointerBinding");
+  CHECK_PARSE_BOOL_FIELD(IncludeStyle.IncludeSortAlphabetically,
+ "IncludeSortAlphabetically");
   CHECK_PARSE_BOOL(DisableFormat);
   CHECK_PARSE_BOOL(IndentCaseLabels);
   CHECK_PARSE_BOOL(IndentCaseBlocks);
Index: clang/lib/Format/Format.cpp
===
--- clang/lib/Format/Format.cpp
+++ clang/lib/Format/Format.cpp
@@ -556,6 +556,8 @@
 IO.mapOptional("IncludeIsMainRegex", Style.IncludeStyle.IncludeIsMainRegex);
 IO.mapOptional("IncludeIsMainSourceRegex",
Style.IncludeStyle.IncludeIsMainSourceRegex);
+IO.mapOptional("IncludeSortAlphabetically",
+   Style.IncludeStyle.IncludeSortAlphabetically);
 IO.mapOptional("IndentCaseLabels", Style.IndentCaseLabels);
 IO.mapOptional("IndentCaseBlocks", Style.IndentCaseBlocks);
 IO.mapOptional("IndentGotoLabels", Style.IndentGotoLabels);
@@ -925,6 +927,7 @@
   {".*", 1, 0, false}};
   LLVMStyle.IncludeStyle.IncludeIsMainRegex = "(Test)?$";
   LLVMStyle.IncludeStyle.IncludeBlocks = tooling::IncludeStyle::IBS_Preserve;
+  LLVMStyle.IncludeStyle.IncludeSortAlphabetically = false;
   LLVMStyle.IndentCaseLabels = false;
   LLVMStyle.IndentCaseBlocks = false;
   LLVMStyle.IndentGotoLabels = true;
@@ -2179,10 +2182,23 @@
   for (unsigned i = 0, e = Includes.size(); i != e; ++i) {
 Indices.push_back(i);
   }
-  llvm::stable_sort(Indices, [&](unsigned LHSI, unsigned RHSI) {
-return std::tie(Includes[LHSI].Priority, Includes[LHSI].Filename) <
-   std::tie(Includes[RHSI].Priority, Includes[RHSI].Filename);
-  });
+
+  if (Style.IncludeStyle.IncludeSortAlphabetically) {
+llvm::stable_sort(Indices, [&](unsigned LHSI, unsigned RHSI) {
+  const auto LHSFilenameLower = Includes[LHSI].Filename.lower();
+  const auto RHSFilenameLower = Includes[RHSI].Filename.lower();
+  return std::tie(Includes[LHSI].Priority, LHSFilenameLower,
+  Includes[LHSI].Filename) <
+ std::tie(Includes[RHSI].Priority, RHSFilenameLower,
+  Includes[RHSI].Filename);
+});
+  } else {
+llvm::stable_sort(Indices, [&](unsigned LHSI, unsigned RHSI) {
+  r

[PATCH] D95017: [clang-format] add case aware include sorting

2021-01-25 Thread Kent Sommer via Phabricator via cfe-commits 
kentsommer added inline comments.



Comment at: clang/docs/ClangFormatStyleOptions.rst:2286
+**IncludeSortAlphabetically** (``bool``)
+  Specify if sorting should be done in an alphabetical and
+  case sensitive fashion.

MyDeveloperDay wrote:
> Are you sure `IncludeSortAlphabetically` expresses what you mean? Once these 
> things get released they cannot be changed easily.
> 
> If you were not sure of a new option, in my view we should slow down and make 
> sure we have the correct design, for example you could have used a enum and 
> it might have given you more possibility for greater flexibility
> 
> ```
> enum IncludeSort
> {
>CaseInsensitive
>CaseSensitive
> }
> ```
> 
> Please give people time to re-review your changes before we commit, 
> especially if they've taken the time to look at your review in the first 
> place. Just saying.
> 
Hi, @MyDeveloperDay I definitely agree. It was not my intention to rush through 
the review. I was simply trying to follow the process outlined in 
https://llvm.org/docs/Contributing.html#how-to-submit-a-patch which mentions 
giving sufficient information to allow for a commit on your behalf when you 
don't have access after an LGTM (which is all that I did). As you can see from 
the lack of additional comments from my end, I was happy to let this sit and be 
reviewed. 

Per the discussion about the option itself, I do believe 
`IncludeSortAlphabetically` currently expresses what I mean as the behavior 
with this off is indeed not an alphabetical sort as case takes precedence over 
the alphabetical ordering. However, looking at the enum and realizing that 
others probably will have additional styles they prefer (maybe they want 
alphabetical but lower case first, etc.) I do believe it might have been a 
better way to go as it leaves more flexibility and room for additional ordering 
styles. Given that this just landed, I would be happy to open a patch to turn 
this into an `enum` as I do see benefits to doing so. What do you think?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D95017/new/

https://reviews.llvm.org/D95017

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

[PATCH] D95017: [clang-format] add case aware include sorting

2021-01-25 Thread Kent Sommer via Phabricator via cfe-commits 
kentsommer added a comment.

In D95017#2522063 , @curdeius wrote:

> Should we then revert ASAP and rework it later? @mydeveloperday

Reverting would also be fine with me. I am happy to drive towards a feature 
(and flags) that everyone is comfortable with in whichever way makes the most 
sense.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D95017/new/

https://reviews.llvm.org/D95017

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

[PATCH] D95017: [clang-format] add case aware include sorting

2021-01-28 Thread Kent Sommer via Phabricator via cfe-commits 
kentsommer updated this revision to Diff 319784.
kentsommer added a comment.

Turn SortIncludes into an enum


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D95017/new/

https://reviews.llvm.org/D95017

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/Format.cpp
  clang/tools/clang-format/ClangFormat.cpp
  clang/unittests/Format/FormatTest.cpp
  clang/unittests/Format/SortImportsTestJava.cpp
  clang/unittests/Format/SortIncludesTest.cpp

Index: clang/unittests/Format/SortIncludesTest.cpp
===
--- clang/unittests/Format/SortIncludesTest.cpp
+++ clang/unittests/Format/SortIncludesTest.cpp
@@ -269,7 +269,7 @@
 }
 
 TEST_F(SortIncludesTest, IncludeSortingCanBeDisabled) {
-  FmtStyle.SortIncludes = false;
+  FmtStyle.SortIncludes = FormatStyle::SI_Never;
   EXPECT_EQ("#include \"a.h\"\n"
 "#include \"c.h\"\n"
 "#include \"b.h\"\n",
@@ -598,6 +598,49 @@
  "a.cc"));
 }
 
+TEST_F(SortIncludesTest, SupportOptionalCaseSensitiveSorting) {
+  EXPECT_FALSE(FmtStyle.SortIncludes == FormatStyle::SI_CaseSensitive);
+
+  FmtStyle.SortIncludes = FormatStyle::SI_CaseSensitive;
+
+  EXPECT_EQ("#include \"A/B.h\"\n"
+"#include \"A/b.h\"\n"
+"#include \"a/b.h\"\n"
+"#include \"B/A.h\"\n"
+"#include \"B/a.h\"\n",
+sort("#include \"B/a.h\"\n"
+ "#include \"B/A.h\"\n"
+ "#include \"A/B.h\"\n"
+ "#include \"a/b.h\"\n"
+ "#include \"A/b.h\"\n",
+ "a.h"));
+
+  Style.IncludeBlocks = clang::tooling::IncludeStyle::IBS_Regroup;
+  Style.IncludeCategories = {
+  {"^\"", 1, 0, false}, {"^<.*\\.h>$", 2, 0, false}, {"^<", 3, 0, false}};
+
+  StringRef UnsortedCode = "#include \"qt.h\"\n"
+   "#include \n"
+   "#include \n"
+   "#include \n"
+   "#include \n"
+   "#include \"vlib.h\"\n"
+   "#include \"Vlib.h\"\n"
+   "#include \"AST.h\"\n";
+
+  EXPECT_EQ("#include \"AST.h\"\n"
+"#include \"qt.h\"\n"
+"#include \"Vlib.h\"\n"
+"#include \"vlib.h\"\n"
+"\n"
+"#include \n"
+"#include \n"
+"\n"
+"#include \n"
+"#include \n",
+sort(UnsortedCode));
+}
+
 TEST_F(SortIncludesTest, SupportCaseInsensitiveMatching) {
   // Setup an regex for main includes so we can cover those as well.
   Style.IncludeIsMainRegex = "([-_](test|unittest))?$";
Index: clang/unittests/Format/SortImportsTestJava.cpp
===
--- clang/unittests/Format/SortImportsTestJava.cpp
+++ clang/unittests/Format/SortImportsTestJava.cpp
@@ -32,7 +32,7 @@
   SortImportsTestJava() {
 FmtStyle = getGoogleStyle(FormatStyle::LK_Java);
 FmtStyle.JavaImportGroups = {"com.test", "org", "com"};
-FmtStyle.SortIncludes = true;
+FmtStyle.SortIncludes = FormatStyle::SI_CaseInsensitive;
   }
 };
 
Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -14406,7 +14406,6 @@
   CHECK_PARSE_BOOL(ObjCSpaceBeforeProtocolList);
   CHECK_PARSE_BOOL(Cpp11BracedListStyle);
   CHECK_PARSE_BOOL(ReflowComments);
-  CHECK_PARSE_BOOL(SortIncludes);
   CHECK_PARSE_BOOL(SortUsingDeclarations);
   CHECK_PARSE_BOOL(SpacesInParentheses);
   CHECK_PARSE_BOOL(SpacesInSquareBrackets);
@@ -14828,6 +14827,16 @@
   CHECK_PARSE("IncludeIsMainSourceRegex: 'abc$'",
   IncludeStyle.IncludeIsMainSourceRegex, "abc$");
 
+  Style.SortIncludes = FormatStyle::SI_Never;
+  CHECK_PARSE("SortIncludes: true", SortIncludes,
+  FormatStyle::SI_CaseInsensitive);
+  CHECK_PARSE("SortIncludes: false", SortIncludes, FormatStyle::SI_Never);
+  CHECK_PARSE("SortIncludes: CaseInsensitive", SortIncludes,
+  FormatStyle::SI_CaseInsensitive);
+  CHECK_PARSE("SortIncludes: CaseSensitive", SortIncludes,
+  FormatStyle::SI_CaseSensitive);
+  CHECK_PARSE("SortIncludes: Never", SortIncludes, FormatStyle::SI_Never);
+
   Style.RawStringFormats.clear();
   std::vector ExpectedRawStringFormats = {
   {
@@ -16819,7 +16828,7 @@
 "#include \"b.h\"\n")});
 
   format::FormatStyle Style = format::getLLVMStyle();
-  Style.SortIncludes = true;
+  Style.SortIncludes = FormatStyle::SI_CaseInsensitive;
   auto FormattedReplaces = formatReplacements(Code, Replaces, Style);
   EXPECT_TRUE(static_cast(FormattedReplaces))
   << llvm::toString(FormattedReplaces.takeError()) << "\n";
Index: clang/tools/clang-format/ClangFormat.cpp
==

[PATCH] D95017: [clang-format] add case aware include sorting

2021-01-28 Thread Kent Sommer via Phabricator via cfe-commits 
kentsommer marked an inline comment as done.
kentsommer added inline comments.



Comment at: clang/docs/ClangFormatStyleOptions.rst:2286
+**IncludeSortAlphabetically** (``bool``)
+  Specify if sorting should be done in an alphabetical and
+  case sensitive fashion.

HazardyKnusperkeks wrote:
> MyDeveloperDay wrote:
> > curdeius wrote:
> > > kentsommer wrote:
> > > > MyDeveloperDay wrote:
> > > > > Are you sure `IncludeSortAlphabetically` expresses what you mean? 
> > > > > Once these things get released they cannot be changed easily.
> > > > > 
> > > > > If you were not sure of a new option, in my view we should slow down 
> > > > > and make sure we have the correct design, for example you could have 
> > > > > used a enum and it might have given you more possibility for greater 
> > > > > flexibility
> > > > > 
> > > > > ```
> > > > > enum IncludeSort
> > > > > {
> > > > >CaseInsensitive
> > > > >CaseSensitive
> > > > > }
> > > > > ```
> > > > > 
> > > > > Please give people time to re-review your changes before we commit, 
> > > > > especially if they've taken the time to look at your review in the 
> > > > > first place. Just saying.
> > > > > 
> > > > Hi, @MyDeveloperDay I definitely agree. It was not my intention to rush 
> > > > through the review. I was simply trying to follow the process outlined 
> > > > in https://llvm.org/docs/Contributing.html#how-to-submit-a-patch which 
> > > > mentions giving sufficient information to allow for a commit on your 
> > > > behalf when you don't have access after an LGTM (which is all that I 
> > > > did). As you can see from the lack of additional comments from my end, 
> > > > I was happy to let this sit and be reviewed. 
> > > > 
> > > > Per the discussion about the option itself, I do believe 
> > > > `IncludeSortAlphabetically` currently expresses what I mean as the 
> > > > behavior with this off is indeed not an alphabetical sort as case takes 
> > > > precedence over the alphabetical ordering. However, looking at the enum 
> > > > and realizing that others probably will have additional styles they 
> > > > prefer (maybe they want alphabetical but lower case first, etc.) I do 
> > > > believe it might have been a better way to go as it leaves more 
> > > > flexibility and room for additional ordering styles. Given that this 
> > > > just landed, I would be happy to open a patch to turn this into an 
> > > > `enum` as I do see benefits to doing so. What do you think?
> > > Hmmm, and how about using the existing option `SortIncludes` and change 
> > > its type from `bool` to some `enum`?
> > > We could then, for backward-compatibility, map `false` to (tentative) 
> > > `Never` and `true` to `ASCII`/`CaseInsensitive`, and add `CaseSensitive`.
> > > 
> > > This will have the advantage of not adding additional style options.
> > > ... and it will prove once again that using `bool`s for style options is 
> > > not a good idea.
> > I think that is an excellent idea @curdeius 
> I also fully support that! (Although I would not say a bool is per se bad.)
@curdeius @MyDeveloperDay @HazardyKnusperkeks this is now done.

However... The command-line option (`--sort-includes`) is not in a place that I 
like at the moment but I am having trouble thinking of any really good options. 

The issue as it stands is that there are a lot of usages of the flag that 
assume it is a `bool` and therefore sometimes do not pass any value. These of 
course could be updated along with the flag to accept a `std::string` value, 
however, this breaks backward capability for anyone relying on that flag not 
requiring a value. As I have it now, backward compatibility is maintained but 
the command line flag is rather severely limited compared to the configuration 
option. Thoughts on which path to take? A third option I have not considered? 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D95017/new/

https://reviews.llvm.org/D95017

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

[PATCH] D95017: [clang-format] add case aware include sorting

2021-01-28 Thread Kent Sommer via Phabricator via cfe-commits 
kentsommer updated this revision to Diff 319786.
kentsommer added a comment.

Remove improper enum mapping


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D95017/new/

https://reviews.llvm.org/D95017

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/Format.cpp
  clang/tools/clang-format/ClangFormat.cpp
  clang/unittests/Format/FormatTest.cpp
  clang/unittests/Format/SortImportsTestJava.cpp
  clang/unittests/Format/SortIncludesTest.cpp

Index: clang/unittests/Format/SortIncludesTest.cpp
===
--- clang/unittests/Format/SortIncludesTest.cpp
+++ clang/unittests/Format/SortIncludesTest.cpp
@@ -269,7 +269,7 @@
 }
 
 TEST_F(SortIncludesTest, IncludeSortingCanBeDisabled) {
-  FmtStyle.SortIncludes = false;
+  FmtStyle.SortIncludes = FormatStyle::SI_Never;
   EXPECT_EQ("#include \"a.h\"\n"
 "#include \"c.h\"\n"
 "#include \"b.h\"\n",
@@ -598,6 +598,49 @@
  "a.cc"));
 }
 
+TEST_F(SortIncludesTest, SupportOptionalCaseSensitiveSorting) {
+  EXPECT_FALSE(FmtStyle.SortIncludes == FormatStyle::SI_CaseSensitive);
+
+  FmtStyle.SortIncludes = FormatStyle::SI_CaseSensitive;
+
+  EXPECT_EQ("#include \"A/B.h\"\n"
+"#include \"A/b.h\"\n"
+"#include \"a/b.h\"\n"
+"#include \"B/A.h\"\n"
+"#include \"B/a.h\"\n",
+sort("#include \"B/a.h\"\n"
+ "#include \"B/A.h\"\n"
+ "#include \"A/B.h\"\n"
+ "#include \"a/b.h\"\n"
+ "#include \"A/b.h\"\n",
+ "a.h"));
+
+  Style.IncludeBlocks = clang::tooling::IncludeStyle::IBS_Regroup;
+  Style.IncludeCategories = {
+  {"^\"", 1, 0, false}, {"^<.*\\.h>$", 2, 0, false}, {"^<", 3, 0, false}};
+
+  StringRef UnsortedCode = "#include \"qt.h\"\n"
+   "#include \n"
+   "#include \n"
+   "#include \n"
+   "#include \n"
+   "#include \"vlib.h\"\n"
+   "#include \"Vlib.h\"\n"
+   "#include \"AST.h\"\n";
+
+  EXPECT_EQ("#include \"AST.h\"\n"
+"#include \"qt.h\"\n"
+"#include \"Vlib.h\"\n"
+"#include \"vlib.h\"\n"
+"\n"
+"#include \n"
+"#include \n"
+"\n"
+"#include \n"
+"#include \n",
+sort(UnsortedCode));
+}
+
 TEST_F(SortIncludesTest, SupportCaseInsensitiveMatching) {
   // Setup an regex for main includes so we can cover those as well.
   Style.IncludeIsMainRegex = "([-_](test|unittest))?$";
Index: clang/unittests/Format/SortImportsTestJava.cpp
===
--- clang/unittests/Format/SortImportsTestJava.cpp
+++ clang/unittests/Format/SortImportsTestJava.cpp
@@ -32,7 +32,7 @@
   SortImportsTestJava() {
 FmtStyle = getGoogleStyle(FormatStyle::LK_Java);
 FmtStyle.JavaImportGroups = {"com.test", "org", "com"};
-FmtStyle.SortIncludes = true;
+FmtStyle.SortIncludes = FormatStyle::SI_CaseInsensitive;
   }
 };
 
Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -14406,7 +14406,6 @@
   CHECK_PARSE_BOOL(ObjCSpaceBeforeProtocolList);
   CHECK_PARSE_BOOL(Cpp11BracedListStyle);
   CHECK_PARSE_BOOL(ReflowComments);
-  CHECK_PARSE_BOOL(SortIncludes);
   CHECK_PARSE_BOOL(SortUsingDeclarations);
   CHECK_PARSE_BOOL(SpacesInParentheses);
   CHECK_PARSE_BOOL(SpacesInSquareBrackets);
@@ -14828,6 +14827,16 @@
   CHECK_PARSE("IncludeIsMainSourceRegex: 'abc$'",
   IncludeStyle.IncludeIsMainSourceRegex, "abc$");
 
+  Style.SortIncludes = FormatStyle::SI_Never;
+  CHECK_PARSE("SortIncludes: true", SortIncludes,
+  FormatStyle::SI_CaseInsensitive);
+  CHECK_PARSE("SortIncludes: false", SortIncludes, FormatStyle::SI_Never);
+  CHECK_PARSE("SortIncludes: CaseInsensitive", SortIncludes,
+  FormatStyle::SI_CaseInsensitive);
+  CHECK_PARSE("SortIncludes: CaseSensitive", SortIncludes,
+  FormatStyle::SI_CaseSensitive);
+  CHECK_PARSE("SortIncludes: Never", SortIncludes, FormatStyle::SI_Never);
+
   Style.RawStringFormats.clear();
   std::vector ExpectedRawStringFormats = {
   {
@@ -16819,7 +16828,7 @@
 "#include \"b.h\"\n")});
 
   format::FormatStyle Style = format::getLLVMStyle();
-  Style.SortIncludes = true;
+  Style.SortIncludes = FormatStyle::SI_CaseInsensitive;
   auto FormattedReplaces = formatReplacements(Code, Replaces, Style);
   EXPECT_TRUE(static_cast(FormattedReplaces))
   << llvm::toString(FormattedReplaces.takeError()) << "\n";
Index: clang/tools/clang-format/ClangFormat.cpp

[PATCH] D95017: [clang-format] add case aware include sorting

2021-01-28 Thread Kent Sommer via Phabricator via cfe-commits 
kentsommer added a comment.

In D95017#2527512 , @curdeius wrote:

> Concerning the `--sort-includes` command-line flag. I'd rather keep it as is 
> and, if need be, work on accepting an **optional** string argument.
> Please update release notes.

@curdeius Just to confirm, are you asking for the "commit" message to be 
updated or something else?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D95017/new/

https://reviews.llvm.org/D95017

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

[PATCH] D95017: [clang-format] add case aware include sorting

2021-01-28 Thread Kent Sommer via Phabricator via cfe-commits 
kentsommer updated this revision to Diff 319804.
kentsommer added a comment.

Added release notes, updated commit message and summary


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D95017/new/

https://reviews.llvm.org/D95017

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/Format.cpp
  clang/tools/clang-format/ClangFormat.cpp
  clang/unittests/Format/FormatTest.cpp
  clang/unittests/Format/SortImportsTestJava.cpp
  clang/unittests/Format/SortIncludesTest.cpp

Index: clang/unittests/Format/SortIncludesTest.cpp
===
--- clang/unittests/Format/SortIncludesTest.cpp
+++ clang/unittests/Format/SortIncludesTest.cpp
@@ -269,7 +269,7 @@
 }
 
 TEST_F(SortIncludesTest, IncludeSortingCanBeDisabled) {
-  FmtStyle.SortIncludes = false;
+  FmtStyle.SortIncludes = FormatStyle::SI_Never;
   EXPECT_EQ("#include \"a.h\"\n"
 "#include \"c.h\"\n"
 "#include \"b.h\"\n",
@@ -598,6 +598,49 @@
  "a.cc"));
 }
 
+TEST_F(SortIncludesTest, SupportOptionalCaseSensitiveSorting) {
+  EXPECT_FALSE(FmtStyle.SortIncludes == FormatStyle::SI_CaseSensitive);
+
+  FmtStyle.SortIncludes = FormatStyle::SI_CaseSensitive;
+
+  EXPECT_EQ("#include \"A/B.h\"\n"
+"#include \"A/b.h\"\n"
+"#include \"a/b.h\"\n"
+"#include \"B/A.h\"\n"
+"#include \"B/a.h\"\n",
+sort("#include \"B/a.h\"\n"
+ "#include \"B/A.h\"\n"
+ "#include \"A/B.h\"\n"
+ "#include \"a/b.h\"\n"
+ "#include \"A/b.h\"\n",
+ "a.h"));
+
+  Style.IncludeBlocks = clang::tooling::IncludeStyle::IBS_Regroup;
+  Style.IncludeCategories = {
+  {"^\"", 1, 0, false}, {"^<.*\\.h>$", 2, 0, false}, {"^<", 3, 0, false}};
+
+  StringRef UnsortedCode = "#include \"qt.h\"\n"
+   "#include \n"
+   "#include \n"
+   "#include \n"
+   "#include \n"
+   "#include \"vlib.h\"\n"
+   "#include \"Vlib.h\"\n"
+   "#include \"AST.h\"\n";
+
+  EXPECT_EQ("#include \"AST.h\"\n"
+"#include \"qt.h\"\n"
+"#include \"Vlib.h\"\n"
+"#include \"vlib.h\"\n"
+"\n"
+"#include \n"
+"#include \n"
+"\n"
+"#include \n"
+"#include \n",
+sort(UnsortedCode));
+}
+
 TEST_F(SortIncludesTest, SupportCaseInsensitiveMatching) {
   // Setup an regex for main includes so we can cover those as well.
   Style.IncludeIsMainRegex = "([-_](test|unittest))?$";
Index: clang/unittests/Format/SortImportsTestJava.cpp
===
--- clang/unittests/Format/SortImportsTestJava.cpp
+++ clang/unittests/Format/SortImportsTestJava.cpp
@@ -32,7 +32,7 @@
   SortImportsTestJava() {
 FmtStyle = getGoogleStyle(FormatStyle::LK_Java);
 FmtStyle.JavaImportGroups = {"com.test", "org", "com"};
-FmtStyle.SortIncludes = true;
+FmtStyle.SortIncludes = FormatStyle::SI_CaseInsensitive;
   }
 };
 
Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -15463,7 +15463,6 @@
   CHECK_PARSE_BOOL(ObjCSpaceBeforeProtocolList);
   CHECK_PARSE_BOOL(Cpp11BracedListStyle);
   CHECK_PARSE_BOOL(ReflowComments);
-  CHECK_PARSE_BOOL(SortIncludes);
   CHECK_PARSE_BOOL(SortUsingDeclarations);
   CHECK_PARSE_BOOL(SpacesInParentheses);
   CHECK_PARSE_BOOL(SpacesInSquareBrackets);
@@ -15959,6 +15958,16 @@
   CHECK_PARSE("IncludeIsMainSourceRegex: 'abc$'",
   IncludeStyle.IncludeIsMainSourceRegex, "abc$");
 
+  Style.SortIncludes = FormatStyle::SI_Never;
+  CHECK_PARSE("SortIncludes: true", SortIncludes,
+  FormatStyle::SI_CaseInsensitive);
+  CHECK_PARSE("SortIncludes: false", SortIncludes, FormatStyle::SI_Never);
+  CHECK_PARSE("SortIncludes: CaseInsensitive", SortIncludes,
+  FormatStyle::SI_CaseInsensitive);
+  CHECK_PARSE("SortIncludes: CaseSensitive", SortIncludes,
+  FormatStyle::SI_CaseSensitive);
+  CHECK_PARSE("SortIncludes: Never", SortIncludes, FormatStyle::SI_Never);
+
   Style.RawStringFormats.clear();
   std::vector ExpectedRawStringFormats = {
   {
@@ -17950,7 +17959,7 @@
 "#include \"b.h\"\n")});
 
   format::FormatStyle Style = format::getLLVMStyle();
-  Style.SortIncludes = true;
+  Style.SortIncludes = FormatStyle::SI_CaseInsensitive;
   auto FormattedReplaces = formatReplacements(Code, Replaces, Style);
   EXPECT_TRUE(static_cast(FormattedReplaces))
   << llvm::toString(FormattedReplaces.takeError()) << "\n";
Index: clan

[PATCH] D95017: [clang-format] add case aware include sorting

2021-01-28 Thread Kent Sommer via Phabricator via cfe-commits 
kentsommer updated this revision to Diff 319806.
kentsommer added a comment.

Fixed release notes typo


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D95017/new/

https://reviews.llvm.org/D95017

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/Format.cpp
  clang/tools/clang-format/ClangFormat.cpp
  clang/unittests/Format/FormatTest.cpp
  clang/unittests/Format/SortImportsTestJava.cpp
  clang/unittests/Format/SortIncludesTest.cpp

Index: clang/unittests/Format/SortIncludesTest.cpp
===
--- clang/unittests/Format/SortIncludesTest.cpp
+++ clang/unittests/Format/SortIncludesTest.cpp
@@ -269,7 +269,7 @@
 }
 
 TEST_F(SortIncludesTest, IncludeSortingCanBeDisabled) {
-  FmtStyle.SortIncludes = false;
+  FmtStyle.SortIncludes = FormatStyle::SI_Never;
   EXPECT_EQ("#include \"a.h\"\n"
 "#include \"c.h\"\n"
 "#include \"b.h\"\n",
@@ -598,6 +598,49 @@
  "a.cc"));
 }
 
+TEST_F(SortIncludesTest, SupportOptionalCaseSensitiveSorting) {
+  EXPECT_FALSE(FmtStyle.SortIncludes == FormatStyle::SI_CaseSensitive);
+
+  FmtStyle.SortIncludes = FormatStyle::SI_CaseSensitive;
+
+  EXPECT_EQ("#include \"A/B.h\"\n"
+"#include \"A/b.h\"\n"
+"#include \"a/b.h\"\n"
+"#include \"B/A.h\"\n"
+"#include \"B/a.h\"\n",
+sort("#include \"B/a.h\"\n"
+ "#include \"B/A.h\"\n"
+ "#include \"A/B.h\"\n"
+ "#include \"a/b.h\"\n"
+ "#include \"A/b.h\"\n",
+ "a.h"));
+
+  Style.IncludeBlocks = clang::tooling::IncludeStyle::IBS_Regroup;
+  Style.IncludeCategories = {
+  {"^\"", 1, 0, false}, {"^<.*\\.h>$", 2, 0, false}, {"^<", 3, 0, false}};
+
+  StringRef UnsortedCode = "#include \"qt.h\"\n"
+   "#include \n"
+   "#include \n"
+   "#include \n"
+   "#include \n"
+   "#include \"vlib.h\"\n"
+   "#include \"Vlib.h\"\n"
+   "#include \"AST.h\"\n";
+
+  EXPECT_EQ("#include \"AST.h\"\n"
+"#include \"qt.h\"\n"
+"#include \"Vlib.h\"\n"
+"#include \"vlib.h\"\n"
+"\n"
+"#include \n"
+"#include \n"
+"\n"
+"#include \n"
+"#include \n",
+sort(UnsortedCode));
+}
+
 TEST_F(SortIncludesTest, SupportCaseInsensitiveMatching) {
   // Setup an regex for main includes so we can cover those as well.
   Style.IncludeIsMainRegex = "([-_](test|unittest))?$";
Index: clang/unittests/Format/SortImportsTestJava.cpp
===
--- clang/unittests/Format/SortImportsTestJava.cpp
+++ clang/unittests/Format/SortImportsTestJava.cpp
@@ -32,7 +32,7 @@
   SortImportsTestJava() {
 FmtStyle = getGoogleStyle(FormatStyle::LK_Java);
 FmtStyle.JavaImportGroups = {"com.test", "org", "com"};
-FmtStyle.SortIncludes = true;
+FmtStyle.SortIncludes = FormatStyle::SI_CaseInsensitive;
   }
 };
 
Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -15463,7 +15463,6 @@
   CHECK_PARSE_BOOL(ObjCSpaceBeforeProtocolList);
   CHECK_PARSE_BOOL(Cpp11BracedListStyle);
   CHECK_PARSE_BOOL(ReflowComments);
-  CHECK_PARSE_BOOL(SortIncludes);
   CHECK_PARSE_BOOL(SortUsingDeclarations);
   CHECK_PARSE_BOOL(SpacesInParentheses);
   CHECK_PARSE_BOOL(SpacesInSquareBrackets);
@@ -15959,6 +15958,16 @@
   CHECK_PARSE("IncludeIsMainSourceRegex: 'abc$'",
   IncludeStyle.IncludeIsMainSourceRegex, "abc$");
 
+  Style.SortIncludes = FormatStyle::SI_Never;
+  CHECK_PARSE("SortIncludes: true", SortIncludes,
+  FormatStyle::SI_CaseInsensitive);
+  CHECK_PARSE("SortIncludes: false", SortIncludes, FormatStyle::SI_Never);
+  CHECK_PARSE("SortIncludes: CaseInsensitive", SortIncludes,
+  FormatStyle::SI_CaseInsensitive);
+  CHECK_PARSE("SortIncludes: CaseSensitive", SortIncludes,
+  FormatStyle::SI_CaseSensitive);
+  CHECK_PARSE("SortIncludes: Never", SortIncludes, FormatStyle::SI_Never);
+
   Style.RawStringFormats.clear();
   std::vector ExpectedRawStringFormats = {
   {
@@ -17950,7 +17959,7 @@
 "#include \"b.h\"\n")});
 
   format::FormatStyle Style = format::getLLVMStyle();
-  Style.SortIncludes = true;
+  Style.SortIncludes = FormatStyle::SI_CaseInsensitive;
   auto FormattedReplaces = formatReplacements(Code, Replaces, Style);
   EXPECT_TRUE(static_cast(FormattedReplaces))
   << llvm::toString(FormattedReplaces.takeError()) << "\n";
Index: clang/tools/clang-format/ClangForma

[PATCH] D95017: [clang-format] add case aware include sorting

2021-01-28 Thread Kent Sommer via Phabricator via cfe-commits 
kentsommer updated this revision to Diff 319819.
kentsommer added a comment.

NFC Rebase


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D95017/new/

https://reviews.llvm.org/D95017

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/Format.cpp
  clang/tools/clang-format/ClangFormat.cpp
  clang/unittests/Format/FormatTest.cpp
  clang/unittests/Format/SortImportsTestJava.cpp
  clang/unittests/Format/SortIncludesTest.cpp

Index: clang/unittests/Format/SortIncludesTest.cpp
===
--- clang/unittests/Format/SortIncludesTest.cpp
+++ clang/unittests/Format/SortIncludesTest.cpp
@@ -269,7 +269,7 @@
 }
 
 TEST_F(SortIncludesTest, IncludeSortingCanBeDisabled) {
-  FmtStyle.SortIncludes = false;
+  FmtStyle.SortIncludes = FormatStyle::SI_Never;
   EXPECT_EQ("#include \"a.h\"\n"
 "#include \"c.h\"\n"
 "#include \"b.h\"\n",
@@ -598,6 +598,49 @@
  "a.cc"));
 }
 
+TEST_F(SortIncludesTest, SupportOptionalCaseSensitiveSorting) {
+  EXPECT_FALSE(FmtStyle.SortIncludes == FormatStyle::SI_CaseSensitive);
+
+  FmtStyle.SortIncludes = FormatStyle::SI_CaseSensitive;
+
+  EXPECT_EQ("#include \"A/B.h\"\n"
+"#include \"A/b.h\"\n"
+"#include \"a/b.h\"\n"
+"#include \"B/A.h\"\n"
+"#include \"B/a.h\"\n",
+sort("#include \"B/a.h\"\n"
+ "#include \"B/A.h\"\n"
+ "#include \"A/B.h\"\n"
+ "#include \"a/b.h\"\n"
+ "#include \"A/b.h\"\n",
+ "a.h"));
+
+  Style.IncludeBlocks = clang::tooling::IncludeStyle::IBS_Regroup;
+  Style.IncludeCategories = {
+  {"^\"", 1, 0, false}, {"^<.*\\.h>$", 2, 0, false}, {"^<", 3, 0, false}};
+
+  StringRef UnsortedCode = "#include \"qt.h\"\n"
+   "#include \n"
+   "#include \n"
+   "#include \n"
+   "#include \n"
+   "#include \"vlib.h\"\n"
+   "#include \"Vlib.h\"\n"
+   "#include \"AST.h\"\n";
+
+  EXPECT_EQ("#include \"AST.h\"\n"
+"#include \"qt.h\"\n"
+"#include \"Vlib.h\"\n"
+"#include \"vlib.h\"\n"
+"\n"
+"#include \n"
+"#include \n"
+"\n"
+"#include \n"
+"#include \n",
+sort(UnsortedCode));
+}
+
 TEST_F(SortIncludesTest, SupportCaseInsensitiveMatching) {
   // Setup an regex for main includes so we can cover those as well.
   Style.IncludeIsMainRegex = "([-_](test|unittest))?$";
Index: clang/unittests/Format/SortImportsTestJava.cpp
===
--- clang/unittests/Format/SortImportsTestJava.cpp
+++ clang/unittests/Format/SortImportsTestJava.cpp
@@ -32,7 +32,7 @@
   SortImportsTestJava() {
 FmtStyle = getGoogleStyle(FormatStyle::LK_Java);
 FmtStyle.JavaImportGroups = {"com.test", "org", "com"};
-FmtStyle.SortIncludes = true;
+FmtStyle.SortIncludes = FormatStyle::SI_CaseInsensitive;
   }
 };
 
Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -15463,7 +15463,6 @@
   CHECK_PARSE_BOOL(ObjCSpaceBeforeProtocolList);
   CHECK_PARSE_BOOL(Cpp11BracedListStyle);
   CHECK_PARSE_BOOL(ReflowComments);
-  CHECK_PARSE_BOOL(SortIncludes);
   CHECK_PARSE_BOOL(SortUsingDeclarations);
   CHECK_PARSE_BOOL(SpacesInParentheses);
   CHECK_PARSE_BOOL(SpacesInSquareBrackets);
@@ -15959,6 +15958,16 @@
   CHECK_PARSE("IncludeIsMainSourceRegex: 'abc$'",
   IncludeStyle.IncludeIsMainSourceRegex, "abc$");
 
+  Style.SortIncludes = FormatStyle::SI_Never;
+  CHECK_PARSE("SortIncludes: true", SortIncludes,
+  FormatStyle::SI_CaseInsensitive);
+  CHECK_PARSE("SortIncludes: false", SortIncludes, FormatStyle::SI_Never);
+  CHECK_PARSE("SortIncludes: CaseInsensitive", SortIncludes,
+  FormatStyle::SI_CaseInsensitive);
+  CHECK_PARSE("SortIncludes: CaseSensitive", SortIncludes,
+  FormatStyle::SI_CaseSensitive);
+  CHECK_PARSE("SortIncludes: Never", SortIncludes, FormatStyle::SI_Never);
+
   Style.RawStringFormats.clear();
   std::vector ExpectedRawStringFormats = {
   {
@@ -17950,7 +17959,7 @@
 "#include \"b.h\"\n")});
 
   format::FormatStyle Style = format::getLLVMStyle();
-  Style.SortIncludes = true;
+  Style.SortIncludes = FormatStyle::SI_CaseInsensitive;
   auto FormattedReplaces = formatReplacements(Code, Replaces, Style);
   EXPECT_TRUE(static_cast(FormattedReplaces))
   << llvm::toString(FormattedReplaces.takeError()) << "\n";
Index: clang/tools/clang-format/ClangFormat.cpp

[PATCH] D95017: [clang-format] Add case aware include sorting.

2021-01-28 Thread Kent Sommer via Phabricator via cfe-commits 
kentsommer updated this revision to Diff 320007.
kentsommer added a comment.

NFC rebase to fix CI


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D95017/new/

https://reviews.llvm.org/D95017

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/Format.cpp
  clang/tools/clang-format/ClangFormat.cpp
  clang/unittests/Format/FormatTest.cpp
  clang/unittests/Format/SortImportsTestJava.cpp
  clang/unittests/Format/SortIncludesTest.cpp

Index: clang/unittests/Format/SortIncludesTest.cpp
===
--- clang/unittests/Format/SortIncludesTest.cpp
+++ clang/unittests/Format/SortIncludesTest.cpp
@@ -269,7 +269,7 @@
 }
 
 TEST_F(SortIncludesTest, IncludeSortingCanBeDisabled) {
-  FmtStyle.SortIncludes = false;
+  FmtStyle.SortIncludes = FormatStyle::SI_Never;
   EXPECT_EQ("#include \"a.h\"\n"
 "#include \"c.h\"\n"
 "#include \"b.h\"\n",
@@ -598,6 +598,49 @@
  "a.cc"));
 }
 
+TEST_F(SortIncludesTest, SupportOptionalCaseSensitiveSorting) {
+  EXPECT_FALSE(FmtStyle.SortIncludes == FormatStyle::SI_CaseSensitive);
+
+  FmtStyle.SortIncludes = FormatStyle::SI_CaseSensitive;
+
+  EXPECT_EQ("#include \"A/B.h\"\n"
+"#include \"A/b.h\"\n"
+"#include \"a/b.h\"\n"
+"#include \"B/A.h\"\n"
+"#include \"B/a.h\"\n",
+sort("#include \"B/a.h\"\n"
+ "#include \"B/A.h\"\n"
+ "#include \"A/B.h\"\n"
+ "#include \"a/b.h\"\n"
+ "#include \"A/b.h\"\n",
+ "a.h"));
+
+  Style.IncludeBlocks = clang::tooling::IncludeStyle::IBS_Regroup;
+  Style.IncludeCategories = {
+  {"^\"", 1, 0, false}, {"^<.*\\.h>$", 2, 0, false}, {"^<", 3, 0, false}};
+
+  StringRef UnsortedCode = "#include \"qt.h\"\n"
+   "#include \n"
+   "#include \n"
+   "#include \n"
+   "#include \n"
+   "#include \"vlib.h\"\n"
+   "#include \"Vlib.h\"\n"
+   "#include \"AST.h\"\n";
+
+  EXPECT_EQ("#include \"AST.h\"\n"
+"#include \"qt.h\"\n"
+"#include \"Vlib.h\"\n"
+"#include \"vlib.h\"\n"
+"\n"
+"#include \n"
+"#include \n"
+"\n"
+"#include \n"
+"#include \n",
+sort(UnsortedCode));
+}
+
 TEST_F(SortIncludesTest, SupportCaseInsensitiveMatching) {
   // Setup an regex for main includes so we can cover those as well.
   Style.IncludeIsMainRegex = "([-_](test|unittest))?$";
Index: clang/unittests/Format/SortImportsTestJava.cpp
===
--- clang/unittests/Format/SortImportsTestJava.cpp
+++ clang/unittests/Format/SortImportsTestJava.cpp
@@ -32,7 +32,7 @@
   SortImportsTestJava() {
 FmtStyle = getGoogleStyle(FormatStyle::LK_Java);
 FmtStyle.JavaImportGroups = {"com.test", "org", "com"};
-FmtStyle.SortIncludes = true;
+FmtStyle.SortIncludes = FormatStyle::SI_CaseInsensitive;
   }
 };
 
Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -15463,7 +15463,6 @@
   CHECK_PARSE_BOOL(ObjCSpaceBeforeProtocolList);
   CHECK_PARSE_BOOL(Cpp11BracedListStyle);
   CHECK_PARSE_BOOL(ReflowComments);
-  CHECK_PARSE_BOOL(SortIncludes);
   CHECK_PARSE_BOOL(SortUsingDeclarations);
   CHECK_PARSE_BOOL(SpacesInParentheses);
   CHECK_PARSE_BOOL(SpacesInSquareBrackets);
@@ -15959,6 +15958,16 @@
   CHECK_PARSE("IncludeIsMainSourceRegex: 'abc$'",
   IncludeStyle.IncludeIsMainSourceRegex, "abc$");
 
+  Style.SortIncludes = FormatStyle::SI_Never;
+  CHECK_PARSE("SortIncludes: true", SortIncludes,
+  FormatStyle::SI_CaseInsensitive);
+  CHECK_PARSE("SortIncludes: false", SortIncludes, FormatStyle::SI_Never);
+  CHECK_PARSE("SortIncludes: CaseInsensitive", SortIncludes,
+  FormatStyle::SI_CaseInsensitive);
+  CHECK_PARSE("SortIncludes: CaseSensitive", SortIncludes,
+  FormatStyle::SI_CaseSensitive);
+  CHECK_PARSE("SortIncludes: Never", SortIncludes, FormatStyle::SI_Never);
+
   Style.RawStringFormats.clear();
   std::vector ExpectedRawStringFormats = {
   {
@@ -17950,7 +17959,7 @@
 "#include \"b.h\"\n")});
 
   format::FormatStyle Style = format::getLLVMStyle();
-  Style.SortIncludes = true;
+  Style.SortIncludes = FormatStyle::SI_CaseInsensitive;
   auto FormattedReplaces = formatReplacements(Code, Replaces, Style);
   EXPECT_TRUE(static_cast(FormattedReplaces))
   << llvm::toString(FormattedReplaces.takeError()) << "\n";
Index: clang/tools/clang-format/ClangFormat.cp

[PATCH] D97927: [clang-format] Rename case sorting

2021-03-05 Thread Kent Sommer via Phabricator via cfe-commits 
kentsommer accepted this revision.
kentsommer added a comment.

LGTM

Sorry for the slow response!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D97927/new/

https://reviews.llvm.org/D97927

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

[PATCH] D94287: [clang-format] Fix include sorting bug

2021-01-07 Thread Kent Sommer via Phabricator via cfe-commits 
kentsommer created this revision.
kentsommer added a project: clang-format.
kentsommer requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

- Fixes a bug that caused `// clang-format off` in a includes section to 
disabled include sorting even after a `// clang-format on` was found.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D94287

Files:
  clang/lib/Format/Format.cpp
  clang/test/Format/line-disable-include-sorting.cpp


Index: clang/test/Format/line-disable-include-sorting.cpp
===
--- /dev/null
+++ clang/test/Format/line-disable-include-sorting.cpp
@@ -0,0 +1,13 @@
+// RUN: grep -Ev "// *[A-Z-]+:" %s \
+// RUN:   | clang-format -style="{BasedOnStyle: LLVM, SortIncludes: true}" \
+// RUN:   | FileCheck -strict-whitespace %s
+// CHECK: {{^//\ clang-format\ off$}}
+// CHECK-NEXT: {{^#include\ $}}
+// CHECK-NEXT: {{^//\ clang-format\ on$}}
+// CHECK-NEXT: {{^#include\ $}}
+// CHECK-NEXT: {{^#include\ $}}
+// clang-format off
+#include 
+// clang-format on
+#include 
+#include 
Index: clang/lib/Format/Format.cpp
===
--- clang/lib/Format/Format.cpp
+++ clang/lib/Format/Format.cpp
@@ -2307,8 +2307,8 @@
 IncludesInBlock.clear();
 FirstIncludeBlock = false;
   }
-  Prev = Pos + 1;
 }
+Prev = Pos + 1;
 if (Pos == StringRef::npos || Pos + 1 == Code.size())
   break;
 SearchFrom = Pos + 1;


Index: clang/test/Format/line-disable-include-sorting.cpp
===
--- /dev/null
+++ clang/test/Format/line-disable-include-sorting.cpp
@@ -0,0 +1,13 @@
+// RUN: grep -Ev "// *[A-Z-]+:" %s \
+// RUN:   | clang-format -style="{BasedOnStyle: LLVM, SortIncludes: true}" \
+// RUN:   | FileCheck -strict-whitespace %s
+// CHECK: {{^//\ clang-format\ off$}}
+// CHECK-NEXT: {{^#include\ $}}
+// CHECK-NEXT: {{^//\ clang-format\ on$}}
+// CHECK-NEXT: {{^#include\ $}}
+// CHECK-NEXT: {{^#include\ $}}
+// clang-format off
+#include 
+// clang-format on
+#include 
+#include 
Index: clang/lib/Format/Format.cpp
===
--- clang/lib/Format/Format.cpp
+++ clang/lib/Format/Format.cpp
@@ -2307,8 +2307,8 @@
 IncludesInBlock.clear();
 FirstIncludeBlock = false;
   }
-  Prev = Pos + 1;
 }
+Prev = Pos + 1;
 if (Pos == StringRef::npos || Pos + 1 == Code.size())
   break;
 SearchFrom = Pos + 1;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D94287: [clang-format] Fix include sorting bug

2021-01-08 Thread Kent Sommer via Phabricator via cfe-commits 
kentsommer abandoned this revision.
kentsommer added a comment.

In D94287#2486099 , @curdeius wrote:

> Seems to be a duplicate of D94206 .

Right you are! Thanks for catching that! I'll abandon this one in favor of the 
one that has already been reviewed.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D94287/new/

https://reviews.llvm.org/D94287

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

[PATCH] D95017: [clang-format] add case aware include sorting

2021-01-19 Thread Kent Sommer via Phabricator via cfe-commits 
kentsommer created this revision.
kentsommer requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

- Adds an option to [clang-format] which sorts headers in an alphabetical 
manner using case only for tie-breakers. The options is off by default in favor 
of the current ASCIIbetical sorting style.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D95017

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/include/clang/Tooling/Inclusions/IncludeStyle.h
  clang/lib/Format/Format.cpp
  clang/unittests/Format/FormatTest.cpp
  clang/unittests/Format/SortIncludesTest.cpp

Index: clang/unittests/Format/SortIncludesTest.cpp
===
--- clang/unittests/Format/SortIncludesTest.cpp
+++ clang/unittests/Format/SortIncludesTest.cpp
@@ -598,6 +598,22 @@
  "a.cc"));
 }
 
+TEST_F(SortIncludesTest, SupportOptionalCaseAwareSorting) {
+  Style.IncludeSortCaseAware = true;
+
+  EXPECT_EQ("#include \"A/B.h\"\n"
+"#include \"A/b.h\"\n"
+"#include \"a/b.h\"\n"
+"#include \"B/A.h\"\n"
+"#include \"B/a.h\"\n",
+sort("#include \"B/a.h\"\n"
+ "#include \"B/A.h\"\n"
+ "#include \"A/B.h\"\n"
+ "#include \"a/b.h\"\n"
+ "#include \"A/b.h\"\n",
+ "a.cc"));
+}
+
 TEST_F(SortIncludesTest, SupportCaseInsensitiveMatching) {
   // Setup an regex for main includes so we can cover those as well.
   Style.IncludeIsMainRegex = "([-_](test|unittest))?$";
Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -14827,6 +14827,8 @@
   "abc$");
   CHECK_PARSE("IncludeIsMainSourceRegex: 'abc$'",
   IncludeStyle.IncludeIsMainSourceRegex, "abc$");
+  CHECK_PARSE("IncludeSortCaseAware: true", IncludeStyle.IncludeSortCaseAware,
+  true);
 
   Style.RawStringFormats.clear();
   std::vector ExpectedRawStringFormats = {
Index: clang/lib/Format/Format.cpp
===
--- clang/lib/Format/Format.cpp
+++ clang/lib/Format/Format.cpp
@@ -556,6 +556,8 @@
 IO.mapOptional("IncludeIsMainRegex", Style.IncludeStyle.IncludeIsMainRegex);
 IO.mapOptional("IncludeIsMainSourceRegex",
Style.IncludeStyle.IncludeIsMainSourceRegex);
+IO.mapOptional("IncludeSortCaseAware",
+   Style.IncludeStyle.IncludeSortCaseAware);
 IO.mapOptional("IndentCaseLabels", Style.IndentCaseLabels);
 IO.mapOptional("IndentCaseBlocks", Style.IndentCaseBlocks);
 IO.mapOptional("IndentGotoLabels", Style.IndentGotoLabels);
@@ -2180,6 +2182,14 @@
 Indices.push_back(i);
   }
   llvm::stable_sort(Indices, [&](unsigned LHSI, unsigned RHSI) {
+if (Style.IncludeStyle.IncludeSortCaseAware) {
+  const auto LHSFilenameLower = Includes[LHSI].Filename.lower();
+  const auto RHSFilenameLower = Includes[RHSI].Filename.lower();
+  return std::tie(Includes[LHSI].Priority, LHSFilenameLower,
+  Includes[LHSI].Filename) <
+ std::tie(Includes[RHSI].Priority, RHSFilenameLower,
+  Includes[RHSI].Filename);
+}
 return std::tie(Includes[LHSI].Priority, Includes[LHSI].Filename) <
std::tie(Includes[RHSI].Priority, Includes[RHSI].Filename);
   });
Index: clang/include/clang/Tooling/Inclusions/IncludeStyle.h
===
--- clang/include/clang/Tooling/Inclusions/IncludeStyle.h
+++ clang/include/clang/Tooling/Inclusions/IncludeStyle.h
@@ -147,6 +147,33 @@
   /// ``ClassImpl.hpp`` would not have the main include file put on top
   /// before any other include.
   std::string IncludeIsMainSourceRegex;
+
+  /// Specify if sorting should be done in a case aware fashion.
+  ///
+  /// By default, clang-format sorts includes in an ASCIIbetical
+  /// fashion. For instance:
+  ///
+  /// \code
+  ///#include "B/a.h"   into  #include "A/B.h"
+  ///#include "B/A.h" #include "A/b.h"
+  ///#include "A/B.h" #include "B/A.h"
+  ///#include "a/b.h" #include "B/a.h"
+  ///#include "A/b.h" #include "a/b.h"
+  /// \endcode
+  ///
+  /// This option will cause includes to be sorted alphabetically with case
+  /// used as a tie-breaker. For instance:
+  ///
+  /// \code
+  ///#include "B/a.h"   into  #include "A/B.h"
+  ///#include "B/A.h" #include "A/b.h"
+  ///#include "A/B.h" #include "a/b.h"
+  ///#include "a/b.h" #include "B/A.h"
+  ///#include "A/b.h" #include "B/a.h"

[PATCH] D95017: [clang-format] add case aware include sorting

2021-01-19 Thread Kent Sommer via Phabricator via cfe-commits 
kentsommer added a comment.

I'll work on fixing the unit tests. Thought they ran with check-cpang-format 
but I was obviously wrong.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D95017/new/

https://reviews.llvm.org/D95017

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

[PATCH] D95017: [clang-format] add case aware include sorting

2021-01-20 Thread Kent Sommer via Phabricator via cfe-commits 
kentsommer updated this revision to Diff 317790.
kentsommer added a comment.

Fix unittests

Fixes both failing unit tests


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D95017/new/

https://reviews.llvm.org/D95017

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/include/clang/Tooling/Inclusions/IncludeStyle.h
  clang/lib/Format/Format.cpp
  clang/unittests/Format/FormatTest.cpp
  clang/unittests/Format/SortIncludesTest.cpp

Index: clang/unittests/Format/SortIncludesTest.cpp
===
--- clang/unittests/Format/SortIncludesTest.cpp
+++ clang/unittests/Format/SortIncludesTest.cpp
@@ -598,6 +598,22 @@
  "a.cc"));
 }
 
+TEST_F(SortIncludesTest, SupportOptionalCaseAwareSorting) {
+  Style.IncludeSortCaseAware = true;
+
+  EXPECT_EQ("#include \"A/B.h\"\n"
+"#include \"A/b.h\"\n"
+"#include \"a/b.h\"\n"
+"#include \"B/A.h\"\n"
+"#include \"B/a.h\"\n",
+sort("#include \"B/a.h\"\n"
+ "#include \"B/A.h\"\n"
+ "#include \"A/B.h\"\n"
+ "#include \"a/b.h\"\n"
+ "#include \"A/b.h\"\n",
+ "a.h"));
+}
+
 TEST_F(SortIncludesTest, SupportCaseInsensitiveMatching) {
   // Setup an regex for main includes so we can cover those as well.
   Style.IncludeIsMainRegex = "([-_](test|unittest))?$";
Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -14395,6 +14395,8 @@
   CHECK_PARSE_BOOL(DeriveLineEnding);
   CHECK_PARSE_BOOL(DerivePointerAlignment);
   CHECK_PARSE_BOOL_FIELD(DerivePointerAlignment, "DerivePointerBinding");
+  CHECK_PARSE_BOOL_FIELD(IncludeStyle.IncludeSortCaseAware,
+ "IncludeSortCaseAware");
   CHECK_PARSE_BOOL(DisableFormat);
   CHECK_PARSE_BOOL(IndentCaseLabels);
   CHECK_PARSE_BOOL(IndentCaseBlocks);
Index: clang/lib/Format/Format.cpp
===
--- clang/lib/Format/Format.cpp
+++ clang/lib/Format/Format.cpp
@@ -556,6 +556,8 @@
 IO.mapOptional("IncludeIsMainRegex", Style.IncludeStyle.IncludeIsMainRegex);
 IO.mapOptional("IncludeIsMainSourceRegex",
Style.IncludeStyle.IncludeIsMainSourceRegex);
+IO.mapOptional("IncludeSortCaseAware",
+   Style.IncludeStyle.IncludeSortCaseAware);
 IO.mapOptional("IndentCaseLabels", Style.IndentCaseLabels);
 IO.mapOptional("IndentCaseBlocks", Style.IndentCaseBlocks);
 IO.mapOptional("IndentGotoLabels", Style.IndentGotoLabels);
@@ -2180,6 +2182,14 @@
 Indices.push_back(i);
   }
   llvm::stable_sort(Indices, [&](unsigned LHSI, unsigned RHSI) {
+if (Style.IncludeStyle.IncludeSortCaseAware) {
+  const auto LHSFilenameLower = Includes[LHSI].Filename.lower();
+  const auto RHSFilenameLower = Includes[RHSI].Filename.lower();
+  return std::tie(Includes[LHSI].Priority, LHSFilenameLower,
+  Includes[LHSI].Filename) <
+ std::tie(Includes[RHSI].Priority, RHSFilenameLower,
+  Includes[RHSI].Filename);
+}
 return std::tie(Includes[LHSI].Priority, Includes[LHSI].Filename) <
std::tie(Includes[RHSI].Priority, Includes[RHSI].Filename);
   });
Index: clang/include/clang/Tooling/Inclusions/IncludeStyle.h
===
--- clang/include/clang/Tooling/Inclusions/IncludeStyle.h
+++ clang/include/clang/Tooling/Inclusions/IncludeStyle.h
@@ -147,6 +147,33 @@
   /// ``ClassImpl.hpp`` would not have the main include file put on top
   /// before any other include.
   std::string IncludeIsMainSourceRegex;
+
+  /// Specify if sorting should be done in a case aware fashion.
+  ///
+  /// By default, clang-format sorts includes in an ASCIIbetical
+  /// fashion. For instance:
+  ///
+  /// \code
+  ///#include "B/a.h"   into  #include "A/B.h"
+  ///#include "B/A.h" #include "A/b.h"
+  ///#include "A/B.h" #include "B/A.h"
+  ///#include "a/b.h" #include "B/a.h"
+  ///#include "A/b.h" #include "a/b.h"
+  /// \endcode
+  ///
+  /// This option will cause includes to be sorted alphabetically with case
+  /// used as a tie-breaker. For instance:
+  ///
+  /// \code
+  ///#include "B/a.h"   into  #include "A/B.h"
+  ///#include "B/A.h" #include "A/b.h"
+  ///#include "A/B.h" #include "a/b.h"
+  ///#include "a/b.h" #include "B/A.h"
+  ///#include "A/b.h" #include "B/a.h"
+  /// \endcode
+  ///
+  /// This option is off by default.
+  bool IncludeSortCaseAware;
 };

[PATCH] D95017: [clang-format] add case aware include sorting

2021-01-20 Thread Kent Sommer via Phabricator via cfe-commits 
kentsommer updated this revision to Diff 318111.
kentsommer added a comment.

Expanded unit test


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D95017/new/

https://reviews.llvm.org/D95017

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/include/clang/Tooling/Inclusions/IncludeStyle.h
  clang/lib/Format/Format.cpp
  clang/unittests/Format/FormatTest.cpp
  clang/unittests/Format/SortIncludesTest.cpp

Index: clang/unittests/Format/SortIncludesTest.cpp
===
--- clang/unittests/Format/SortIncludesTest.cpp
+++ clang/unittests/Format/SortIncludesTest.cpp
@@ -598,6 +598,47 @@
  "a.cc"));
 }
 
+TEST_F(SortIncludesTest, SupportOptionalCaseAwareSorting) {
+  Style.IncludeSortCaseAware = true;
+
+  EXPECT_EQ("#include \"A/B.h\"\n"
+"#include \"A/b.h\"\n"
+"#include \"a/b.h\"\n"
+"#include \"B/A.h\"\n"
+"#include \"B/a.h\"\n",
+sort("#include \"B/a.h\"\n"
+ "#include \"B/A.h\"\n"
+ "#include \"A/B.h\"\n"
+ "#include \"a/b.h\"\n"
+ "#include \"A/b.h\"\n",
+ "a.h"));
+
+  Style.IncludeBlocks = clang::tooling::IncludeStyle::IBS_Regroup;
+  Style.IncludeCategories = {
+  {"^\"", 1, 0, false}, {"^<.*\\.h>$", 2, 0, false}, {"^<", 3, 0, false}};
+
+  StringRef UnsortedCode = "#include \"qt.h\"\n"
+   "#include \n"
+   "#include \n"
+   "#include \n"
+   "#include \n"
+   "#include \"vlib.h\"\n"
+   "#include \"Vlib.h\"\n"
+   "#include \"AST.h\"\n";
+
+  EXPECT_EQ("#include \"AST.h\"\n"
+"#include \"qt.h\"\n"
+"#include \"Vlib.h\"\n"
+"#include \"vlib.h\"\n"
+"\n"
+"#include \n"
+"#include \n"
+"\n"
+"#include \n"
+"#include \n",
+sort(UnsortedCode));
+}
+
 TEST_F(SortIncludesTest, SupportCaseInsensitiveMatching) {
   // Setup an regex for main includes so we can cover those as well.
   Style.IncludeIsMainRegex = "([-_](test|unittest))?$";
Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -14395,6 +14395,8 @@
   CHECK_PARSE_BOOL(DeriveLineEnding);
   CHECK_PARSE_BOOL(DerivePointerAlignment);
   CHECK_PARSE_BOOL_FIELD(DerivePointerAlignment, "DerivePointerBinding");
+  CHECK_PARSE_BOOL_FIELD(IncludeStyle.IncludeSortCaseAware,
+ "IncludeSortCaseAware");
   CHECK_PARSE_BOOL(DisableFormat);
   CHECK_PARSE_BOOL(IndentCaseLabels);
   CHECK_PARSE_BOOL(IndentCaseBlocks);
Index: clang/lib/Format/Format.cpp
===
--- clang/lib/Format/Format.cpp
+++ clang/lib/Format/Format.cpp
@@ -556,6 +556,8 @@
 IO.mapOptional("IncludeIsMainRegex", Style.IncludeStyle.IncludeIsMainRegex);
 IO.mapOptional("IncludeIsMainSourceRegex",
Style.IncludeStyle.IncludeIsMainSourceRegex);
+IO.mapOptional("IncludeSortCaseAware",
+   Style.IncludeStyle.IncludeSortCaseAware);
 IO.mapOptional("IndentCaseLabels", Style.IndentCaseLabels);
 IO.mapOptional("IndentCaseBlocks", Style.IndentCaseBlocks);
 IO.mapOptional("IndentGotoLabels", Style.IndentGotoLabels);
@@ -2180,6 +2182,14 @@
 Indices.push_back(i);
   }
   llvm::stable_sort(Indices, [&](unsigned LHSI, unsigned RHSI) {
+if (Style.IncludeStyle.IncludeSortCaseAware) {
+  const auto LHSFilenameLower = Includes[LHSI].Filename.lower();
+  const auto RHSFilenameLower = Includes[RHSI].Filename.lower();
+  return std::tie(Includes[LHSI].Priority, LHSFilenameLower,
+  Includes[LHSI].Filename) <
+ std::tie(Includes[RHSI].Priority, RHSFilenameLower,
+  Includes[RHSI].Filename);
+}
 return std::tie(Includes[LHSI].Priority, Includes[LHSI].Filename) <
std::tie(Includes[RHSI].Priority, Includes[RHSI].Filename);
   });
Index: clang/include/clang/Tooling/Inclusions/IncludeStyle.h
===
--- clang/include/clang/Tooling/Inclusions/IncludeStyle.h
+++ clang/include/clang/Tooling/Inclusions/IncludeStyle.h
@@ -147,6 +147,33 @@
   /// ``ClassImpl.hpp`` would not have the main include file put on top
   /// before any other include.
   std::string IncludeIsMainSourceRegex;
+
+  /// Specify if sorting should be done in a case aware fashion.
+  ///
+  /// By default, clang-format sorts includes in an ASCIIbetical
+  /// fashion. For instance:
+  ///
+  /// \code
+  ///#include "B/a.h"   into  #include "A/B.h"
+  ///#includ

[PATCH] D95017: [clang-format] add case aware include sorting

2021-01-20 Thread Kent Sommer via Phabricator via cfe-commits 
kentsommer added a comment.

@HazardyKnusperkeks let me know if the additions to the unit test are along the 
lines you were hoping for or not, I think this captures what you had in mind.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D95017/new/

https://reviews.llvm.org/D95017

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