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

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