clang-format now reorders includes, please let us know if that's useful

[Nico Weber]

Hi,

 

After https://codereview.chromium.org/2669263003 (just landed),

`git cl format` will reorder includes in blocks of #includes

that are not separated by newlines.

This works in almost all cases, but it can break some code, e.g.:

  #include <windows.h>

  #include <shellapi.h>

clang-format will reorder these now, but shellapi.h only compiles if

windows.h was included first. Relying on this is brittle, so replace

code like this with

  #include <windows.h>

  // Must be after windows.h:

  #include <shellapi.h>

Since clang-format doesn't reorder across blocks, this will do the right

thing.

This also means you're still on the hook of putting blocks with user headers,

C++ headers, and C headers in the right order.

This will hopefully replace src/tools/sort-headers.py which contains

some hacky heuristics -- just inserting newlines between includes

when needed (with a comment) seems like a better tradeoff anyhow.

And the automatic integration with `git cl format` is nice.

(clang-format has IncludeIsMainRegex and IncludeCategories for adding

heuristics, but we shouldn't use these, they're too complicated.)

Since this has some the above tradeoffs, this is enabled as an experiment

for now. If you think this is useful, or if it causes you problems, please let

me know. Based on feedback, I'll make a call if we should this enabled in

four weeks or so. (Unless it breaks the world somehow, in that case I'll

revert sooner of course.)

Nico

[Scott Violet]

Thanks Nico! I have long desired this!

-Scott

[Rachel Blum]

Wooohoo! This is awesome, thank you for making it happen!

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
    http://groups.google.com/a/chromium.org/group/chromium-dev

[Daniel Cheng]

While IncludeCategories is pretty complicated, I think it'd be nice to set it up with the same heuristics that sort-includes.py currently uses. Wouldn't that be sufficient to start? As we see breakage, we can keep tweaking this. Or are there include files that will simply never agree on ordering?

Daniel

[Nico Weber]

I feel that "if you don't want includes to be reordered, put them in different blocks" is simpler and easier to understand.

[Daniel Cheng]

I don't want to have to think about the ordering of my system headers though: I want to just add them to the system headers section and run clang-format. Otherwise, I have to build to see if I actually put them in the right order. Since the ordering dependencies between system headers shouldn't change very often (if ever), it seems like something that would be fairly stable over time as well.

Daniel

[Nico Weber]

Imho ordering dependencies between system headers are rare and Windows-only. If there's a dependency, it's surprising and should be called out with a comment anyhow, right? Else clang-format will silently not reorder those lines and appear broken for people who don't know about these deps. So the current approach imho leads to more readable code, and it has a simpler implementation too.

[Scott Hess]

I don't want to have to think about the ordering of my system headers,
either, but unfortunately if they have a required ordering, then they
have a required ordering and you really should expose it. If the goal
is to have them be automatically right, the solution isn't to have the
code-formatting system handle that, because then nobody knows why
their ordering is right. The solution is to require everyone to
include a new header file which does it right in the first place, and
forbid inclusion of the headers which are prone to incorrect usage.

[The argument I'm trying to make is basically that you shouldn't
abstract this kind of dependency out into the build system where it's
impossible to notice, but I feel like it could be phrased better.]

-scott

[Daniel Cheng]

On Thu, Feb 2, 2017 at 4:48 PM Nico Weber <tha...@chromium.org> wrote:

Imho ordering dependencies between system headers are rare and Windows-only. If there's a dependency, it's surprising and should be called out with a comment anyhow, right? Else clang-format will silently not reorder those lines and appear broken for people who don't know about these deps. So the current approach imho leads to more readable code, and it has a simpler implementation too.

There are a couple POSIX network headers that have specific include ordering dependencies as well. I'm just not a fan of all the extra lines this will require. =)

Daniel

[Nico Weber]

On Thu, Feb 2, 2017 at 7:54 PM, Daniel Cheng <dch...@chromium.org> wrote:

On Thu, Feb 2, 2017 at 4:48 PM Nico Weber <tha...@chromium.org> wrote:

Imho ordering dependencies between system headers are rare and Windows-only. If there's a dependency, it's surprising and should be called out with a comment anyhow, right? Else clang-format will silently not reorder those lines and appear broken for people who don't know about these deps. So the current approach imho leads to more readable code, and it has a simpler implementation too.

There are a couple POSIX network headers that have specific include ordering dependencies as well. I'm just not a fan of all the extra lines this will require. =)

My feeling is that this will be very rare. To collect data: If someone adds newlines between blocks, maybe add BUG=688155 on CLs that do so. Then we'll get an idea how common this is.

[Mark Mentovai]

I can mostly get on board with this, but the <windows.h> case is so common that I don’t think it’s worth a comment each time there’s an ordering sensitivity.

Since this case is so common, I think we should also standardize on whether <windows.h> should be in a section by itself above all other system headers, or whether it should go in the same section as most system headers, putting the ones that must follow <windows.h> in a separate section below. Essentially, the choice is between:

#include <windows.h>

#include <stdio.h>

#include <shellapi.h>

#include <time.h>

and

#include <stdio.h>

#include <time.h>

#include <windows.h>

#include <shellapi.h>

I kind of prefer the second form, but the first is probably an easier rule to follow.

[Éric Noyau]

Works with #import in ObjC++ files as well. Awesome. Thanks.

-- Éric

--

[Nico Weber]

That's bad :-) Do you have an example CL where they disagree?

On Fri, Feb 3, 2017 at 12:14 PM, hongchan <hong...@chromium.org> wrote:

I found that there is a conflict:

1) |git cl format| does the reodering for me, but the presubmit check from |git cl upload| complains about the incorrect order.

2) When I revert the ordering manually, then the presubmit check warns me that I need to run |git cl format|.

Certainly I can bypass the step 2), but this needs to be resolved.

Other than that, this looks nice! Thanks!

[Carlos Knippschild]

I found a bug that might be related to this change: I'm getting includes incorrectly ordered (non-alphabetically).

https://bugs.chromium.org/p/chromium/issues/detail?id=691155

--

[Daniel Cheng]

I commented on the bug, but this is because of the rule in https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes:

In dir/foo.cc or dir/foo_test.cc, whose main purpose is to implement or test the stuff in dir2/foo2.h, order your includes as follows:

1. dir2/foo2.h.

2. C system files.

3. C++ system files.

4. Other libraries' .h files.

5. Your project's .h files.

Since mixed_content_navigation_throttle_untittest.cc's main purpose is probably to test mixed_content_navigation_throttle.h, it's ordered at the top.

Daniel