On 08/01/2017 08:25 AM, Darin Adler wrote: >> On Jul 31, 2017, at 2:04 PM, Michael Catanzaro <[email protected]> wrote: >> >> On Mon, Jul 31, 2017 at 9:27 PM, Darin Adler <[email protected]> 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 > [email protected] > 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 _______________________________________________ webkit-dev mailing list [email protected] https://lists.webkit.org/mailman/listinfo/webkit-dev
