Re: [chromium-dev] windows.h and include order

[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

On Mon, Aug 16, 2021 at 11:40 AM 'Bruce Dawson' via Chromium-dev <chromi...@chromium.org> wrote:

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

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

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

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

[Thomas Sepez]

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.

[Mark Mentovai]

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

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

[Thomas Sepez]

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]

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.

[Thomas Sepez]

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.

[K. Moon]

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

[Thomas Sepez]

Haha, thanks! 

[Peter Boström]

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

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

[Thomas Sepez]

Really?  I wouldn't think that. 

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

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

[akbar lalegoon22]

من نه گروه دارم نه عوض میشم هراحمقی سرپرسته یا هرکی فکرمیکنه گروه خانوادگی 

یا هرکوفته دیگه ای که طی این سه ماه بخاطر همکاری اینستاگرام با کلاهبرداران برای دزدیدن اکانت من به  وجود امده شخص ثالث مدیر نهاد یا هرخردیگه ای به من ارتباط نداشته وتوسط هکر ایجادشده 

درضمن من گوکل کروم قبولش ندارم 

اینکه کرومیم هست نمیدونم چه کوفتی بخاطرهمبن هرزنامه میشه 

درضمن هرکس بامن کارداره فارسی تایپ کنه شاید. بخونم ولی اینجوری مستقیم سطل اشعال 

۳ ماه به خاکسیاه نشستم واون هکرحروم زاده شیخی که ادمین فدرال گذاشته اسمه ننه خرابشو 

واون چلغوز هشتک مدیا شکایت مفصلی از دوسه ارگان دردست اقدام هست که یکباره میخوام خیالشون. که راحتشد فکرکنن تموم شده. دزدی تموم درنصف روزچنان اتهامات سیاسی سنگینی بهشون نسبت داده وتایید هم شده همونطورکه اونا بدون اطلاع من وبجای من جلسه میزارن سریک پیج مجازی من. سر زندگیشون  توزندان بپوسن وحتی نفهمن چی شد که هرکدوم ۲۲ سال حبس ویک روزهم بخاطراینکه بخشیده نشه بهشون خورد وسریعا پروندهاشون مثل ماله من ازهمه سیستمها پاک میشه ولی این کجا اون کجا 

در تاریخ جمعه ۲۰ اوت ۲۰۲۱،‏ ۱۱:۰۶ Greg Thompson <g...@chromium.org> نوشت:

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.