kentsommer marked an inline comment as done. 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. ---------------- 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? 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