Proposed include ordering style specification

[Brett Wilson]

Currently we use two styles of doing ifdefs for includes, one with the
includes all alphabetized and the ifdefs around the files that should
only be included on some platforms:

#include "base/basictypes.h"
#if defined(OS_WIN)
#include "base/win_util.h"
#endif
#include "base/utf_string_conversions.h"

And one with all the "special" ones at the bottom:

#include "base/basictypes.h"
#include "base/utf_string_conversions.h"

#if defined(OS_WIN)
#include "base/win_util.h"
#endif

I would like to propose that we standardize on the latter approach, I
find it much easier to read what's going on, and some files are
getting out of control with different platforms included for different
random files (although in some cases this indicates that the file
should really be refactored).

This also solves the ambiguity about where "build/build_config.h"
should be included. Some files put it first, in violation of the C++
style guide's ordering, so that OS_* can be used for the other include
files. With this rule, "build/build_config.h" would always appear in
its normal alphabetical ordering in any file that needs it since
anything depending on it will be after the include block. Any header
files that depend on build_config.h are supposed to be including it
themselves anyway.

Brett

[Sanjeev Radhakrishnan]

While I don't have a strong opinion on the actual style of includes, shouldn't build/build_config.h always be force included (using the /FI option on Visual Studio, don't know the equivalent on other platforms) or the defines specified as preprocessor defines on the command-line to cl? It seems like developers shouldn't have to explicitly include this file.

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

[Peter Kasting]

On Sun, Oct 17, 2010 at 12:44 PM, Brett Wilson <bre...@chromium.org> wrote:

I would like to propose that we standardize on the latter approach

SGTM

PK 

[Brett Wilson]

I went ahead and added this to the style guide. If anybody violently
disagrees, we will have to settle it via an old-style wiki edit war.

Brett

[Sanjeev Radhakrishnan]

Any opinion on force including build/build_config.h? If we need to conditionally include a system header file and need to include build/build_config.h before the system file which is also a violation of the C++ coding style include ordering.

[Mark Mentovai]

Brett Wilson wrote:
> And one with all the "special" ones at the bottom:
>
> #include "base/basictypes.h"
> #include "base/utf_string_conversions.h"
>
> #if defined(OS_WIN)
> #include "base/win_util.h"
> #endif
>

> I would like to propose that we standardize on the latter approach,

Having had experience with both of the forms you recognize, I agree
with taking the “separate conditional #includes” approach.

Sanjeev Radhakrishnan wrote:
> Any opinion on force including build/build_config.h?

Don’t do this. Prefix header injection sucks. The source code should
build as it stands checked-in, and not require some magic forced
header.

[Brett Wilson]

I agree with Mark, I prefer to avoid forced injection if possible.

Brett

[Lei Zhang]

Can we get a clarification on one case? Which of these two rules have
precedent? System headers in front or platform specific headers after?
Either way gcl lint will complain.

I.e. Do you prefer:

#include "build/build_config.h"

#if defined(OS_WIN)
#include <windows.h>
#endif

#include "base/bar.h"
#include "base/foo.h"

#if defined(OS_WIN)
#include "base/qux_win.h"
#endif

or

#include "base/bar.h"
#include "base/foo.h"
#include "build/build_config.h"

#if defined(OS_WIN)
#include <windows.h>
#include "base/qux_win.h"
#endif

On Sun, Oct 17, 2010 at 12:44 PM, Brett Wilson <bre...@chromium.org> wrote:

[Peter Kasting]

On Tue, Dec 14, 2010 at 6:24 PM, Lei Zhang <the...@chromium.org> wrote:

Can we get a clarification on one case? Which of these two rules have
precedent? System headers in front or platform specific headers after?
Either way gcl lint will complain.

I.e. Do you prefer:

#include "build/build_config.h"

#if defined(OS_WIN)
#include <windows.h>
#endif

#include "base/bar.h"
#include "base/foo.h"

#if defined(OS_WIN)
#include "base/qux_win.h"
#endif

or

#include "base/bar.h"
#include "base/foo.h"
#include "build/build_config.h"

#if defined(OS_WIN)
#include <windows.h>
#include "base/qux_win.h"
#endif

The specific case of <windows.h> is special because it's the only header I can think of that frequently has to be before almost everything else in order to work right.  But in general, I thought the consensus on this thread was to use method 2 if possible.

PK 

[Brett Wilson]

I agree with Peter. Method 1 has a problem with build_config.h not
being included before needing some defines. Method 2 will always work
if every file that needs windows.h includes it like it's supposed to,
and allows us to have build/build_config.h in the normal place in each
file.

Bret