windows.h and include order

[Bruce Dawson]

The Chromium C++ style guide says to place platform-specific #includes in their own section below the “normal” #includes. This means that <windows.h> normally ends up near the bottom, in a #if block, and this is non-controversial.

However in Windows-specific source files with no #if for windows.h the desired location is less clear. In a couple of recent changes with windows.h at the bottom I got reviewer push-back on one and a presubmit failure on the other:

c:\src\chromium\src\ui\views\controls\webview\unhandled_keyboard_event_handler_win.cc:9:  Found C system header after other header. Should be: unhandled_keyboard_event_handler_win.h, c system, c++ system, other.  [build/include_order] [4]

I think that windows.h is always a platform-specific header, even in Windows-specific source files. Among other things it defines thousands of macros which can affect our code and this impact is best minimized by including it as late as possible so that its macros don't pollute our headers.

So, I'd like to clarify the Chromium style guide to say that "headers like windows.h" (maybe just windows.h?) should be in their own section at the end, and update the presubmit to allow this. I also need to figure out why the presubmit doesn't fire consistently.

Thoughts on this change/clarification?

--

Bruce Dawson, he/him

[K. Moon]

I think the tooling can only tell system headers by the fact that they use <> #include instead of "" #include. The tooling would have to carve out an exemption for windows.h to put it at the end.

I guess one alternative would be to put platform-specific includes in #if blocks, even when the file is platform-specific. (I don't think this would be popular.) Otherwise, I think the status quo is probably best; maybe clarify the style guide to imply that the platform-specific rules apply only to files that aren't platform-specific?

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups "Chromium-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to chromium-dev...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAE5mQiNau6MYaszhEkWdW47O9tnPhnuJHQS7idgKAV3Dx35i8g%40mail.gmail.com.

[Avi Drissman]

I always read "platform specific header" as an #if header, like this:

#include "a.h"
#include "b.h"

#if defined(OS_WIN)

 

#include <windows.h>

#include "c.h"

 

#endif  // OS_WIN

However, for files that only build on a single platform, I and the Mac folks have always considered Mac platform headers to be like any other C header file for this purpose and put it at the top of everything. Mac headers tend to not make macro definitions, though.

Some clarification is certainly welcome here. +cxx as they're into this kind of question :)

Avi

[K. Moon]

This section of the Chromium style guide (https://chromium.googlesource.com/chromium/src/+/refs/heads/main/styleguide/c++/c++.md#platform_specific-code) also says:

"Repeat the standard #include order within this section."

So the example seems to be treating <windows.h> like a C system header within the section.

You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CACWgwAbyoHo1gbHMOWT7x6detdXAJad9nS-9i_uPenwTTwEWfg%40mail.gmail.com.

[Avi Drissman]

I didn't mean to confuse things with my example. Bruce is specifically asking about source files which are only compiled on one platform.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CACwGi-6MWcVrsg05tYVWb154kU19AqsbXcz-fqVjOxjOObqzrQ%40mail.gmail.com.

[Mark Mentovai]

Bruce Dawson wrote:

I think that windows.h is always a platform-specific header, even in Windows-specific source files. Among other things it defines thousands of macros which can affect our code and this impact is best minimized by including it as late as possible so that its macros don't pollute our headers.

This sounds like a nice goal, but I don’t know how well it scales. As soon as one of our .h files needs to #include <windows.h>, any downstream #including .cc file will pick up <windows.h> earlier than you’re hoping to achieve. The gaggle of Windows macros will appear and disappear during the preprocessing of any of our .h files based on the ordinary maintenance of seemingly unrelated #includes. This is a problem unless we have a rule against #including <windows.h> from our own .h files, and I don’t think that’s realistic.

I don’t like this either (in particular because that one header is pretty polluted), but it’s actually a strong argument in favor of #including <windows.h> among the system headers, before any of our own local headers, to improve consistency and reduce the number of, say, unrelated macro collisions that might otherwise occur as #includes are introduced or reordered.

So, I'd like to clarify the Chromium style guide to say that "headers like windows.h" (maybe just windows.h?) should be in their own section at the end, and update the presubmit to allow this. I also need to figure out why the presubmit doesn't fire consistently.

I recall that a big problem in the past with <windows.h> was that it needed to be #included prior to certain other win32 headers (my top memory is <dbghelp.h>), in a way that caused problems with our typical alphabetic sort, since w follows almost everything else alphabetically. I don’t know if this is still a major problem, but if it is and we choose to offer specific guidance on handling <windows.h>, it should account for this problem too.

[Bruce Dawson]

>  big problem in the past with <windows.h> was that it needed to be #included prior to certain other win32 headers

Yep, still a problem. I've been removing windows.h includes from many headers (not all, but from most of the frequently used ones) and that often exposes .cc files that include windows.h too-late. Here is one recent fix for this.

In that case the bitter realities clearly trump the style guide. If alphabetical order doesn't compile then we need to include windows.h first.

Regarding the macro/name-space pollution, as Mark says, it is inevitable that windows.h will sometimes be included before all of our headers are included, so it's not like we have absolute purity anyway. I have reduced this a lot, but getting it to zero is probably not practical. If the consensus is that windows.h should be at the top in platform-specific source files then I'll roll with that. In that case the style-guide clarification would be that "platform-specific includes" means those in #if defined(OS_*) blocks.

--

Bruce Dawson, he/him

[Robert Liao]

I'd imagine the standard rule applies for files that are compiled only on one platform. My expectation is that <windows.h> would be towards the top for files that are only compiled on Windows.

To unsubscribe from this group and stop receiving emails from it, send an email to chromium-dev+unsubscribe@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAE5mQiNau6MYaszhEkWdW47O9tnPhnuJHQS7idgKAV3Dx35i8g%40mail.gmail.com.

--
You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CACWgwAbyoHo1gbHMOWT7x6detdXAJad9nS-9i_uPenwTTwEWfg%40mail.gmail.com.

--
You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.

[Greg Thompson]

I'm getting dizzy here! :-)

I liked the simplicity of the (very) old guidance of always putting windows.h above all other C system headers when it was needed. Can we go back to that? I find that the current guidance of putting it in alpha order unless it needs to precede some other header leads to more time spent in fixing compile failures and writing feedback in code reviews. Bruce: would your current effort be made simpler if windows.h was always before other C system headers? I suppose not since a.h could include b.h, c.h, then d.h, which included windows.h. I think we should simply --include windows.h so it comes first for every file. :-)

To unsubscribe from this group and stop receiving emails from it, send an email to chromium-dev...@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAE5mQiNau6MYaszhEkWdW47O9tnPhnuJHQS7idgKAV3Dx35i8g%40mail.gmail.com.

--
You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CACWgwAbyoHo1gbHMOWT7x6detdXAJad9nS-9i_uPenwTTwEWfg%40mail.gmail.com.

--
You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CACwGi-6MWcVrsg05tYVWb154kU19AqsbXcz-fqVjOxjOObqzrQ%40mail.gmail.com.

--
You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/a1933f07-120b-40b2-bc64-175ace239ca3n%40chromium.org.

[Mark Mentovai]

In that case, we can make (and maybe even have made) an exception to allow build_config.h to precede windows.h.

On Fri, Aug 20, 2021, 12:44 PM 'Thomas Sepez' via cxx <c...@chromium.org> wrote:

One complication I noticed is that if we want to write

#if defined(OS_WIN)

#include <windows.h>

#endif

don't we first have to #include "build/build_confg.h" to get the OS_WIN definition?  Which would mean that "build_config.h" would have to come before any <foo.h> system headers, contradicting the rule?

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAKQnr0T1Ze0Doyuv-oHj%2B4SxNk-q7OZEBX8nmX2SwuPnWbP-ZQ%40mail.gmail.com.

--
You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAPYU9wRX6%3D3HNENe4ysHQ0ywxQtKuXmVzLG4ZiR04iM8cT387A%40mail.gmail.com.

[K. Moon]

Conditional includes go after all other includes (according to the style guide), so this example wouldn't be a problem. It's only if you want to include windows.h unconditionally that there's an argument about where it should go.

On Fri, Aug 20, 2021 at 11:35 AM Thomas Sepez <tse...@google.com> wrote:

Yes, but then you're back to the "there's one magic file that allowed to go first", and I thought that was completely against the spirit of the rules.

[K. Moon]

That was one of my earlier suggestions, which I predicted wouldn't be popular, but maybe I'm wrong about that. :-)

On Fri, Aug 20, 2021 at 11:54 AM Thomas Sepez <tse...@google.com> wrote:

One could argue that it should always be included conditionally even in win-specific files along the lines of

#if defined(OS_WIN)

#include <windows.h>

#else

#error "Included on wrong platform"

#endif

as a courtesy in diagnosing build failures.

[Peter Boström]

I think a foo_win.cc having #if defined(OS_WIN) anywhere in it is pretty confusing.

On Fri, Aug 20, 2021 at 11:58 AM 'Thomas Sepez' via cxx <c...@chromium.org> wrote:

Haha, thanks! 

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAPYU9wRmXR-f0ztu2eLQjvUrZJYMuU3YFa5npypZB3uwP%3DZ7HA%40mail.gmail.com.

[Peter Boström]

If I saw the proposed if defined(OS_WIN) inside a win.cc file I would probably just remove the conditional and move windows.h back to the top and then not understand why I was in for a world of pain unless this email was fresh in my mind.

Unless it's inside something like this, yeah it would confuse me.

#if !defined(OS_WIN)

#error "This should not be built outside Windows"

#endif  // !defined(OS_WIN)

The above way would also be a really convoluted way compared to something like:

// This needs to be included after other headers/last/whatever, see crbug.com/foo.

#include <Windows.h>

Because you're really using it as a mechanism to make sure it's included later, you should have to be explicit about that.

On Fri, Aug 20, 2021 at 12:02 PM Thomas Sepez <tse...@google.com> wrote:

Really?  I wouldn't think that. 

[Mark Mentovai]

A technique I've used to avoid #if-soup in other cross-platform codebases is to provide a no-op <windows.h> and make it available to non-Windows targets.

Definitely controversial, and I wouldn't push for that in this case, but it's an idea that could ease some of these concerns.

On Fri, Aug 20, 2021, 3:02 PM Thomas Sepez <tse...@google.com> wrote:

Really?  I wouldn't think that. 

On Fri, Aug 20, 2021 at 12:00 PM Peter Boström <pb...@chromium.org> wrote: