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

[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