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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
