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

[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: <code>... When true: <code>".

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