kentsommer 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.
----------------
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?
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
