[PATCH] D95017: [clang-format] add case aware include sorting

MyDeveloperDay via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Mon Jan 25 14:18:19 PST 2021


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



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D95017/new/

https://reviews.llvm.org/D95017



More information about the cfe-commits mailing list