Bug 124849 – check-webkit-style should check for misplaced primary header inside ENABLE(FEATURE)

• Component: Tools / Tests
• Status: NEW

[Brian Burg]

I believe the house style for feature guards is that:

1. Feature-specific headers should contain ENABLE(FEATURE) to wrap most or all of the header, starting after the File_h ifdef and before secondary includes. Conceptually, the header shouldn't define things that are not implemented if the feature is not enabled.

2. Feature-specific implementation files should place the primary header immediately below #include "config.h" and before the ENABLE(FEATURE) ifdef.

And check-webkit-style does not check either. These style rules are not on http://www.webkit.org/coding/coding-style.html but are fairly consistent in newer code.

Header file:

... copyright block ...
#ifndef Feature_h
#define Feature_h

#if ENABLE(FEATURE)

... header includes, namespace, definitions, ...

#endif // ENABLE(FEATURE)

#endif // Feature_h


Implementation file:

#include "config.h"
#include "Feature.h"

#if ENABLE(FEATURE)

... secondary headers ...

#endif // ENABLE(FEATURE)

[Brian Burg]

Darin, is my interpretation of the pre-existing style accurate?

[Darin Adler]

First, the argument for having a feature guard in a header at all:

--------

I believe a long while back we had some discussion of whether to put conditionals into headers or at the include sites in files. I looked in webkit-dev to find the initial discussion but could not find it. I am not sure we got consensus on this.

The arguments for putting the conditionals into headers are: 1) we can include headers without carefully writing conditionals which makes the lists of headers in implementation files tidier, 2) we get compile time errors rather than link time errors if we accidentally use things in a header outside its conditional, 3) it makes it clear to a reader of the header that the contents of the header is tied to a specific feature, and 4) it makes it less likely we will accidentally compile unnecessary code that just happens to link because it’s all in headers (templates, inlines, and such).

The arguments for putting the conditionals into includes in implementation files are: 1) it can make compiles faster due to fewer included headers, and 2) it reduces the chances that someone will remove an include because it’s not used in their configuration, breaking another configuration because they didn’t realize an include was configuration-specific.

--------

Now on to the two issues:

(In reply to comment #0)
> 1. Feature-specific headers should contain ENABLE(FEATURE) to wrap most or all of the header, starting after the File_h ifdef and before secondary includes. Conceptually, the header shouldn't define things that are not implemented if the feature is not enabled.
> 
> 2. Feature-specific implementation files should place the primary header immediately below #include "config.h" and before the ENABLE(FEATURE) ifdef.

I think both of these rules are the best practices for fastest compiling, and also make for clean formatting. We want to compile as little as possible when a header or implementation file is completely turned off.

On reflection, rule (2) is not the best for fastest compiling. Putting the conditional after "config.h" and before the corresponding header include would be faster. I think we tend to follow rule (2) because separating the "config.h" include from the corresponding header include would make the file just "look wrong" to people used to the rest of our code.