kentsommer added inline comments.
================
Comment at: clang/docs/ClangFormatStyleOptions.rst:2039
+**IncludeSortCaseAware** (``bool``)
+ Specify if sorting should be done in a case aware fashion.
----------------
MyDeveloperDay wrote:
> curdeius wrote:
> > The name is somewhat awkward IMO (in the sense it doesn't read nicely), but
> > it's nice to be (alphabetically) closer to other Include-related options in
> > the documentation.
> > I have no better name though. Just noticing.
> I agree how about `IncludeSortCaseSensitive`
I went with `IncludeSortAlphabetically` for now. To me, this reads better and
seems to more clearly indicate the effect of turning this on. The downside is
it no longer captures the fact that case does still play a part in the sort.
What do you both think?
================
Comment at: clang/docs/ClangFormatStyleOptions.rst:2045
+
+ .. code-block:: c++
+
----------------
curdeius wrote:
> I'd write this in the same manner as other options, so sth like "When false:
> <code>... When true: <code>".
>
> Otherwise, it's necessary to read up to the end of the description "This
> option is off by default" and conclude that off=false and that was the first
> snippet that corresponded to false.
Good call, I definitely agree. I've rewritten this to better match the `bool`
flag instances.
================
Comment at: clang/lib/Format/Format.cpp:2185
llvm::stable_sort(Indices, [&](unsigned LHSI, unsigned RHSI) {
+ if (Style.IncludeStyle.IncludeSortCaseAware) {
+ const auto LHSFilenameLower = Includes[LHSI].Filename.lower();
----------------
curdeius wrote:
> Maybe it would be better for CodeGen to get the if outside of sort? It would
> mean some code duplication, but maybe it could be mitigated with a lambda.
This is a good point. I moved the `if` outside of the call to `stable_sort`. I
don't feel there is actually much code duplication caused by this (other than
the call signature).
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
