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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
