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

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

2021-03-02 Thread Chris Johnson via Phabricator via cfe-commits

PragmaNull added a comment.

In D95017#2595230 , 
@HazardyKnusperkeks wrote:

> 



> Do you make a change? Otherwise I will do, but that will take some time, 
> because I'm rather busy.

Sorry, I have not; unfortunately I've been rather busy myself and haven't had a 
moment to get on it.


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-03-01 Thread Björn Schäpers via Phabricator via cfe-commits

HazardyKnusperkeks added a comment.

In D95017#2572578 , @PragmaNull wrote:

> In D95017#2572238 , @curdeius wrote:
>
>> Do you have an idea for better names?
>> I see that e.g. MS documentation uses ascending order and case-sensitive 
>> order.
>
> I'm okay with the names, it just seems to me that they are reversed, where 
> CaseSensitive should sort using ASCIIbetic order, and CaseInsensitive should 
> ignore case entirely in the same way that most case-insensitive string 
> compares would.

Do you make a change? Otherwise I will do, but that will take some time, 
because I'm rather busy.


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-02-19 Thread Björn Schäpers via Phabricator via cfe-commits

HazardyKnusperkeks added a comment.

In D95017#2572578 , @PragmaNull wrote:

> In D95017#2572238 , @curdeius wrote:
>
>> Do you have an idea for better names?
>> I see that e.g. MS documentation uses ascending order and case-sensitive 
>> order.
>
> I'm okay with the names, it just seems to me that they are reversed, where 
> CaseSensitive should sort using ASCIIbetic order, and CaseInsensitive should 
> ignore case entirely in the same way that most case-insensitive string 
> compares would.

After initially reading it I was not sure about it, but now I have looked into 
the wikipedia:

> In computers, case sensitivity defines whether uppercase and lowercase 
> letters are treated as distinct (case-sensitive) or equivalent 
> (case-insensitive).

So I would support a name switch. I would wait a few days for @kentsommer to 
say something, if he doesn't respond just make a diff and add me as reviewer. :)


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-02-18 Thread Chris Johnson via Phabricator via cfe-commits

PragmaNull added a comment.

In D95017#2572238 , @curdeius wrote:

> Do you have an idea for better names?
> I see that e.g. MS documentation uses ascending order and case-sensitive 
> order.

I'm okay with the names, it just seems to me that they are reversed, where 
CaseSensitive should sort using ASCIIbetic order, and CaseInsensitive should 
ignore case entirely in the same way that most case-insensitive string compares 
would.

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-02-18 Thread Marek Kurdej via Phabricator via cfe-commits

curdeius added a comment.

Do you have an idea for better names?
I see that e.g. MS documentation uses ascending order and case-sensitive order.


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-02-18 Thread Chris Johnson via Phabricator via cfe-commits

PragmaNull added a comment.

I find the naming of the case sensitive options confusing here. 
When I read "CaseSensitive" I think of the behavior of strcmp() which sorts 
according to "ASCIIbetical" order. But here "CaseSensitive" throws away case by 
comparing the result of "Filename.lower()" which I would consider 
case-insensitive, or the same behavior as a call to stricmp().


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-02-02 Thread Marek Kurdej via Phabricator via cfe-commits

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGa8105b3766e4: [clang-format] Add case aware include sorting. 
(authored by kentsommer, committed by curdeius).

Changed prior to commit:
  https://reviews.llvm.org/D95017?vs=320007=320771#toc

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 = {
   {
@@ -17970,7 +17979,7 @@
 "#include \"b.h\"\n")});
 
   format::FormatStyle Style = format::getLLVMStyle();
-  Style.SortIncludes = true;
+  Style.SortIncludes =

[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:

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

2021-01-28 Thread MyDeveloperDay via Phabricator via cfe-commits

MyDeveloperDay accepted this revision.
MyDeveloperDay added a comment.

I like this much better LGTM


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 Marek Kurdej via Phabricator via cfe-commits

curdeius accepted this revision.
curdeius added a comment.
This revision is now accepted and ready to land.

LGTM.


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 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 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:

[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:

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

2021-01-28 Thread Marek Kurdej via Phabricator via cfe-commits

curdeius added a comment.

I was thinking of 
https://github.com/llvm/llvm-project/blob/main/clang/docs/ReleaseNotes.rst#clang-format.
 Look at the history to see how it was written before.
Also, you may change the revision title if you want (it will become the commit 
message).


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 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 Marek Kurdej via Phabricator via cfe-commits

curdeius 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.

kentsommer wrote:
> 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? 
> I also fully support that! (Although I would not say a bool is per se bad.)

@HazardyKnusperkeks, I was of course a bit exaggerating :).


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 Marek Kurdej via Phabricator via cfe-commits

curdeius added a comment.

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.


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 Marek Kurdej via Phabricator via cfe-commits

curdeius added inline comments.



Comment at: clang/lib/Format/Format.cpp:399
+IO.enumCase(Value, "false", FormatStyle::SI_Never);
+IO.enumCase(Value, "", FormatStyle::SI_CaseInsensitive);
+IO.enumCase(Value, "true", FormatStyle::SI_CaseInsensitive);

curdeius wrote:
> Is this needed?
Ok. You just fixed it.


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 Marek Kurdej via Phabricator via cfe-commits

curdeius added inline comments.



Comment at: clang/lib/Format/Format.cpp:399
+IO.enumCase(Value, "false", FormatStyle::SI_Never);
+IO.enumCase(Value, "", FormatStyle::SI_CaseInsensitive);
+IO.enumCase(Value, "true", FormatStyle::SI_CaseInsensitive);

Is this needed?


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 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 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-26 Thread Björn Schäpers via Phabricator via cfe-commits

HazardyKnusperkeks 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:
> 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.)


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-26 Thread MyDeveloperDay via Phabricator via cfe-commits

MyDeveloperDay 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.

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 


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-26 Thread Marek Kurdej via Phabricator via cfe-commits

curdeius requested changes to this revision.
curdeius added a comment.
This revision now requires changes to proceed.

Requesting changes so that it appears correctly in the review queue.


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-26 Thread Marek Kurdej via Phabricator via cfe-commits

curdeius 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.

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.


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-26 Thread Marek Kurdej via Phabricator via cfe-commits

curdeius added a comment.

Ok, reverted. If there's a doubt, then I guess it's the best solution.


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-25 Thread Marek Kurdej via Phabricator via cfe-commits

curdeius added a comment.

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


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 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 MyDeveloperDay via Phabricator via cfe-commits

MyDeveloperDay 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.

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.



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 Marek Kurdej via Phabricator via cfe-commits

This revision was automatically updated to reflect the committed changes.
Closed by commit rG3395a336b025: [clang-format] add case aware include sorting 
(authored by kentsommer, committed by curdeius).

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
@@ -15145,6 +15145,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
@@ -571,6 +571,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);
@@ -940,6 +942,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;
@@ -2194,10 +2197,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);
+});
+

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

2021-01-25 Thread Marek Kurdej via Phabricator via cfe-commits

curdeius accepted this revision.
curdeius added a comment.

LGTM. Thanks for being prompt at addressing the comments. I'll merge it today 
for you if @MyDeveloperDay has no more remarks.


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) {
+

[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 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,

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

2021-01-24 Thread MyDeveloperDay via Phabricator via cfe-commits

MyDeveloperDay added inline comments.



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

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`


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 Marek Kurdej via Phabricator via cfe-commits

curdeius added inline comments.



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

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.



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

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.



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();

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.



Comment at: clang/unittests/Format/SortIncludesTest.cpp:602
+TEST_F(SortIncludesTest, SupportOptionalCaseAwareSorting) {
+  Style.IncludeSortCaseAware = true;
+

You should test that this option is false for LLVM style.
BTW, shouldn't you set it somewhere so that the style is explicit?


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-22 Thread Björn Schäpers via Phabricator via cfe-commits

HazardyKnusperkeks added a comment.

I can push that, but will wait a bit longer so the others have time to object.


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-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-21 Thread Björn Schäpers via Phabricator via cfe-commits

HazardyKnusperkeks accepted this revision.
HazardyKnusperkeks added a comment.
This revision is now accepted and ready to land.

LGTM.


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 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

[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"
+  ///

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

2021-01-20 Thread Björn Schäpers via Phabricator via cfe-commits

HazardyKnusperkeks added a comment.

Please expand the test case to different options like grouping. Otherwise looks 
good.


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-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-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

47 matches

Mail list logo