[webkit-dev] What's the rationale for not including config.h in any header files?

[Alicia Boya García]

On 08/01/2017 08:25 AM, Darin Adler wrote:
>> On Jul 31, 2017, at 2:04 PM, Michael Catanzaro <mcatanzaro at igalia.com> wrote:
>>
>> On Mon, Jul 31, 2017 at 9:27 PM, Darin Adler <darin at apple.com> wrote:
>>> I don’t think we should add lots of includes of “config.h”, though. I think we can come up with something better.
>>
>> Like what?
> 
> We should consider reducing the size of “config.h” and only use it for things that it’s really needed for, using more conventional header includes for other purposes.
> 
> Ideally we would eliminate “config.h” entirely and replace it with normal headers.
> 
> I definitely don’t think we need to use the same name, “config.h”, for 9 separate headers that don’t have the same contents in 9 different directories in the overall WebKit source code. A risk of putting includes of “config.h” in header files is that it isn’t clear which of the “config.h” files will be included. If we aren’t obliged to use the name “config.h” because of autotools, then perhaps we can name these more in line with WebKit coding style, something like WebCoreConfiguration.h, but also strive to get rid of them entirely.
> 
> In projects not using autotools and using IDEs to build I often see a header referred to as the “prefix” that is automatically included by the IDE, with no #include statements at all in the source files. That’s used as a sort of substitute for #define statements on the command line and is what I used before I became familiar with autotools.
> 
> I am concerned to see there is also a header named “cmakeconfig.h” so maybe we do need “config.h”.
> 
> — Darin
> _______________________________________________
> webkit-dev mailing list
> webkit-dev at lists.webkit.org
> https://lists.webkit.org/mailman/listinfo/webkit-dev
> 

I'm not sure if it's realistic to get rid of configuration headers.
WebKit development builds use just too many build-time flags to enable
features.

On the other hand, I agree completely in the name issue, which is also
an scoping issue. The situation I found is something like this:

"cmakeconfig.h" is a file generated by CMake with all the flags we
enable at configure time. One reason to do that is precisely because
there are too many to put comfortably in the command line (I count 177
#define's there).

There is a single cmakeconfig.h file for an entire WebKit build. This
makes sense: If a feature is implemented in both WebCore and WebKit you
want it to be enabled either both or in none.

On the other hand, there is currently a config.h file in every of our
sub-project directories: WTF, WebCore, JavaScriptCore, WebKit and
WebDriver. Every config.h file includes cmakeconfig.h, plus some other
stuff. Unfortunately, the kind of "stuff" that is additionally included
or defined varies a lot between sub-projects.

Having several files called config.h is confusing at the very least,
even more when they are included from different sub-projects. The same
#include "config.h" line refers to different files in different contexts.

This is a quite nasty problem, as the author of a header file in a
sub-project may assume it's safe to rely on something being defined
because it's included in that sub-project's config.h file... and most of
the time it will work, but it may not be the case when that file is
included (even transitively) by any other file from other sub-project
that uses a different config.h file.

As a consequence, quirks often have to be added to config.h files of
projects with dependencies. For instance, JavaScriptCore/config.h
includes "runtime/JSExportMacros.h" (and therefore many headers in JSC
assume it is included) and the same happens in WebCore, whose config.h
includes "WebCore/PlatformExportMacros.h". The WebKit sub-project
includes headers from both, so its config.h has to include both
"runtime/JSExportMacros.h" and "WebCore/PlatformExport.h" too in order
to build.

Complicating things a bit more there is
ThirdParty/gtest/include/gtest/internal/gtest-port.h. In the gtest
compilation context there is no config.h in path, but it uses WTF and
JSC files nevertheless, and some of these actually check for
configuration flags:

#include <wtf/Platform.h>
#include <wtf/ExportMacros.h>
#include <runtime/JSExportMacros.h>
#include <wtf/Assertions.h>
#include <wtf/FastMalloc.h>



There are a few things all config.h files agree:

* Include cmakeconfig.h if a CMake build is being used.
* Include wtf/FastMalloc.h.
* Include wtf/DisallowCType.h.

Other than that, the differences are mostly sub-project specific quirks
to get around compilation issues of the sub-project. The case of the
WebKit sub-project is a bit more worrying as it has many defaults for
flags there, but they would not be read from the context of other
sub-projects.

We should consider why we have several config.h files and if that makes
sense or there are better alternatives. In principle, since config.h is
about configuration flags, and there is a single global set of flags for
the entire project, it would make much more sense to have a single true
config.h instead of sub-project-specific ones.

This new one true config.h file should not only load the flags from
cmakeconfig.h, but also set defaults for missing variables. As far as I
know this is currently partly done in wtf/Platform.h and partly done in
WebKit/config.h, but that's not ideal. There should be no way to shoot
yourself in the foot by including a header file with missing or
incorrect flags.

Renaming this file as WebKitConfig.h would be even better as it would
reduce the chances or collisions with other projects, although it would
add a bit of friction to patch merges in the short term.


— Alicia