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