[PATCH] D67843: DisableFormat also now disables SortIncludes

Mon Sep 23 11:51:19 PDT 2019

SamMaier added a comment.

In D67843#1677983 <https://reviews.llvm.org/D67843#1677983>, @MyDeveloperDay wrote:

> I assume the intention was that users could have DisableFormat=true and SortIncludes=true when they want to sort the includes but not perform any additional formatting in the code.
>
> I think by making this change you make it impossible to run clang-format through a codebase with the sole intention of just sorting the headers. (which I could see as potentially useful isolated functionality)..
>
> If SortIncludes is false by default? (which you are making it not for no style so I'm unclear what it would be now if you running without a BasedOnStyle setting (uninitialized?)) then you don't need to supply both unless you are using LLVM style or one of the other styles that turn it on.
>
> Are you sure this is the right change?

This is from my experience with Chromium's use of clang-format.

People intuitively use DisableFormat: true to (they think) turn off formatting for that subdirectory. The Chromium project has 10 instances of this (https://cs.chromium.org/search/?q=file:.clang-format+DisableFormat:%5CWtrue&sq=package:chromium&type=cs). The "correct" thing to do here is to specify BasedOnStyle: none if they actually want no formatting. If you don't provide a BasedOnStyle, the DefaultFallbackStyle is LLVM, which means that SortIncludes is always true unless directly overridden.

I see 3 options:

1. Take this change as is. This breaks the ability to only sort includes without doing any other formatting. I'm not sure how much sort includes only is used, but my guess would be not often. It makes DisableFormat actually disable all formatting.
2. Don't change anything. This means that DisableFormat does not disable all formatting, despite the docs (https://clang.llvm.org/docs/ClangFormatStyleOptions.html) stating it "disables formatting completely". The only real way to disable formatting for a directory is to use BasedOnStyle: none, or to specify SortIncludes: false, which both are pretty unintuitive IMO.
3. Change the default fallback to something else if there is a .clang-format file with no BasedOnStyle. This could have farther reaching consequences which could affect more users who depend on clang-format just applying LLVM style even if they don't specify it.

Repository:
  rC Clang

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

https://reviews.llvm.org/D67843