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

--

[hongchan]

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!

[Nico Weber]

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

[Nico Weber]

hongchan, is your change in Blink? dcheng points out that "The Blink presubmit requires that system headers in <> come after user headers in "", but clang-format defaults to the opposite."

If so, I think we should just remove that blink presubmit check, and over time system headers will bubble to the top like they are in the rest of chromium. (They're supposed to be in a separate block too, but oh well.)

The other alternative would be to turn this off for third_party/WebKit

If anyone has strong opinions, shout, else I'll delete blink's header ordering presubmit (since this is now enforced by the clang-format presubmit, only slightly differently).

[Hongchan Choi]

Nico,

I had to bypass the presubmit warning to upload the CL. So, I don't have an example, but I think the issue is easily reproducible.

Whatever our resolution is, I am fine as long as it is consistent.

Thanks!

-Hongchan

[Nico Weber]

Was it in blink? If so, I disabled the conflicting headers sorting presubmit there yesterday afternoon.

[Takashi Toyoshima]

I notice this change today on updating my CL.

I'm +1 to use the same rule even in Blink as possible. Then, what is the final ordering rule in Blink so far?

As Nico said, chromium style is "user headers follow system headers AND split system headers in a separate block". Now new "cl format" makes system headers bubble to the top, but it's still in the same block.

Also, I haven't tracked recent style changes in Blink, but can I assume the style guideline in the project page should be up to date, and will be updated for this change?

Anyway, I'd submit today's CL with "cl format" results as is.

--

Takashi Toyoshima
Software Engineer, Google

[Nico Weber]

On Mon, Feb 6, 2017 at 1:01 AM, Takashi Toyoshima <toyo...@chromium.org> wrote:

I notice this change today on updating my CL.

I'm +1 to use the same rule even in Blink as possible. Then, what is the final ordering rule in Blink so far?

As Nico said, chromium style is "user headers follow system headers AND split system headers in a separate block". Now new "cl format" makes system headers bubble to the top, but it's still in the same block.

Also, I haven't tracked recent style changes in Blink, but can I assume the style guideline in the project page should be up to date, and will be updated for this change?

The final rule should be "like in Chromium code" I think. But since the motivation of this change was to save work, so `cl format` output should just be fine imho. If people are bothered by the reorderings in their CL or the lack of the newline between system and user includes , I can reorder all include lines at some point.

I updated the style guide you linked to.

[Daniel Cheng]

One other thing I noticed: usually, Blink includes v8 with #include <v8.h>. Should this be changed to the fully qualified path with #include "v8/include/v8.h"?

Daniel

[Takashi Toyoshima]

On Mon, Feb 6, 2017 at 10:49 PM, Nico Weber <tha...@chromium.org> wrote:

On Mon, Feb 6, 2017 at 1:01 AM, Takashi Toyoshima <toyo...@chromium.org> wrote:

I notice this change today on updating my CL.

I'm +1 to use the same rule even in Blink as possible. Then, what is the final ordering rule in Blink so far?

As Nico said, chromium style is "user headers follow system headers AND split system headers in a separate block". Now new "cl format" makes system headers bubble to the top, but it's still in the same block.

Also, I haven't tracked recent style changes in Blink, but can I assume the style guideline in the project page should be up to date, and will be updated for this change?

The final rule should be "like in Chromium code" I think. But since the motivation of this change was to save work, so `cl format` output should just be fine imho. If people are bothered by the reorderings in their CL or the lack of the newline between system and user includes , I can reorder all include lines at some point.

OK. That makes sense.

 

I updated the style guide you linked to.

Thanks! 

[Takashi Toyoshima]

On Tue, Feb 7, 2017 at 6:49 PM, Daniel Cheng <dch...@chromium.org> wrote:

One other thing I noticed: usually, Blink includes v8 with #include <v8.h>. Should this be changed to the fully qualified path with #include "v8/include/v8.h"?

+1.

Actually, all four kinds exist under the current WebKit directory, <v8.h>, <v8/include/v8.h>, "v8.h", and "v8/include/v8.h"

[Nico Weber]

On Tue, Feb 7, 2017 at 4:49 AM, Daniel Cheng <dch...@chromium.org> wrote:

One other thing I noticed: usually, Blink includes v8 with #include <v8.h>. Should this be changed to the fully qualified path with #include "v8/include/v8.h"?

IMHO yes.

[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