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