Hi, Do we still need separate WebCorePrefix.h and config.h? The former has this comment, which I don't think is true any more: /* This prefix file should contain only: * 1) files to precompile for faster builds * 2) in one case at least: OS-X-specific performance bug workarounds * 3) the special trick to catch us using new or delete without including "config.h" * The project should be able to build without this header, although we rarely test that. */ /* Things that need to be defined globally should go into "config.h". */ There are many things that contradict this comment in this file. And even when precompiled header is not in use, we include WebCorePrefix.h from config.h anyway: // Using CMake with Unix makefiles does not use prefix headers. #if PLATFORM(MAC) && defined(BUILDING_WITH_CMAKE) #include "WebCorePrefix.h" #endif I'm mostly looking at some HAVE and ENABLE macros that are in these and should be elsewhere, but the confusion between these files bothers me a lot. Should we move everything from config.h to WebCorePrefix.h, and only keep config.h just to include WebCorePrefix for the lone build scenario where that's needed, and to undef new/delete? - Alexey
Useful background exists in Wikipedia: https://en.wikipedia.org/wiki/Prefix_header <https://en.wikipedia.org/wiki/Prefix_header> and https://en.wikipedia.org/wiki/Precompiled_header <https://en.wikipedia.org/wiki/Precompiled_header> are relevant. Alexey, perhaps you know all of this already, but here’s how I understand the intention behind having both of these files. The “WebCorePrefix.h” file is a prefix header. We put things here that we want defined everywhere in the project. The “config.h” file is a workaround for build systems that don’t support a prefix header. It’s inspired by the “config.h” files used in build systems based on autoconf, and was originally intended to keep the WebKit project compatible with them. The file is unnecessary for builds systems like Xcode that support prefix headers. Once we decided to use “config.h” as a “pseudo prefix header” we still decided to have a real prefix header under the Xcode build system, at least so we could precompile it. Ideally, under the Xcode build system, including “config.h” should have no effect at all other than participating in the check to help us ensure we didn’t forget to include it for the benefit of other build systems. We wanted to cleverly devise the contents of the prefix header so it’s easy to catch a mistake where we forget to include “config.h” somewhere; we’d like to get compile errors if we forget in most cases. Maybe we don’t need both of these any more. Two possibilities occur to me: - If we don’t need to support systems without support for prefix headers, we can eliminate “config.h”, which would be nice since we can streamline all our source files by removing the include for it. - If we can get fast compilation without precompiling a header, then we don’t need to use a prefix header. However, if we need support for systems without prefix headers and we need to use a prefix header for precompilation, then I do think we need to keep both of these. Their names or contents could change, but I think would still need two separate headers. It would be great to “purify” any strange properties that these headers have accumulated. There should not be meaningful content repeated in both places. Ideally, content that needs to be included everywhere would be in a single place, perhaps a third header appropriately included by these two, and the prefix header and “pseudo prefix header” would just contain the tricks used to check for their proper use. A corollary to this is that we should resist the urge to put platform-specific things into the prefix header just because it happens to be used on those platforms and no others. So perhaps there are Cocoa-specific things in WebCorePrefix.h that should instead be in a common place shared by both. — Darin
On Dec 7, 2018, at 2:44 PM, Alexey Proskuryakov <ap@webkit.org> wrote:
only keep config.h just to include WebCorePrefix for the lone build scenario where that's needed, and to undef new/delete?
I think the answer likely lies here: What is this lone build scenario where config.h is needed? — Darin
On Dec 8, 2018, at 2:56 PM, Darin Adler <darin@apple.com> wrote:
On Dec 7, 2018, at 2:44 PM, Alexey Proskuryakov <ap@webkit.org> wrote:
only keep config.h just to include WebCorePrefix for the lone build scenario where that's needed, and to undef new/delete?
I think the answer likely lies here: What is this lone build scenario where config.h is needed?
I guess it’s CMake with Unix makefiles. Question for our CMake on Unix experts: Can we get CMake with Unix makefiles to support prefix headers? — Darin
OK, here’s my answer after thinking on it a bit: Best would be to eliminate “config.h”: Change “config.h” into an empty file first, then remove all “config.h” includes, and then remove the file. But to do that, we need to make sure every build system for WebKit supports prefix headers. I don’t know how close to that we are. Maybe close? How can we quickly find out? Lacking that, I think we can and should change “config.h” so it’s just an include of “WebCorePrefix.h”, or the other way around. I think it would be valuable to keep the feature where we try to catch cases where we forget to include “config.h”, on the platforms that use a prefix header, for the benefit of the platforms that do not. That might mean small complexity remains and one file won’t literally just be a trivial include of the other. I suppose it’s also important to verify that there is no benefit to precompiling less than all of what “config.h” includes. — Darin PS: I don’t think we know that there is only one configuration that needs “config.h”. That second code snippet from your original message, Alexey, was only relevant for platforms that are trying to support macOS without prefix headers, and there could be any number of non-macOS cases. (And that include seems like a relatively recent change done by someone who didn’t fully understand the original scheme.)
CMake on Mac should not affect the decision here. I added that as a hack in http://trac.webkit.org/r172346 as part of an experimental project that isn’t being used by anyone. If we decide to resume CMake on Mac development and that has moved, we will find a better way to solve the same build problem.
On Dec 8, 2018, at 3:22 PM, Darin Adler <darin@apple.com> wrote:
OK, here’s my answer after thinking on it a bit:
Best would be to eliminate “config.h”: Change “config.h” into an empty file first, then remove all “config.h” includes, and then remove the file. But to do that, we need to make sure every build system for WebKit supports prefix headers. I don’t know how close to that we are. Maybe close? How can we quickly find out?
Lacking that, I think we can and should change “config.h” so it’s just an include of “WebCorePrefix.h”, or the other way around. I think it would be valuable to keep the feature where we try to catch cases where we forget to include “config.h”, on the platforms that use a prefix header, for the benefit of the platforms that do not. That might mean small complexity remains and one file won’t literally just be a trivial include of the other.
I suppose it’s also important to verify that there is no benefit to precompiling less than all of what “config.h” includes.
— Darin
PS: I don’t think we know that there is only one configuration that needs “config.h”. That second code snippet from your original message, Alexey, was only relevant for platforms that are trying to support macOS without prefix headers, and there could be any number of non-macOS cases. (And that include seems like a relatively recent change done by someone who didn’t fully understand the original scheme.) _______________________________________________ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
On Sun, Dec 9, 2018 at 8:22 AM Darin Adler <darin@apple.com> wrote:
Best would be to eliminate “config.h”: Change “config.h” into an empty file first, then remove all “config.h” includes, and then remove the file. But to do that, we need to make sure every build system for WebKit supports prefix headers. I don’t know how close to that we are. Maybe close? How can we quickly find out?
GCC and Clang support '-include <file>' option. https://gcc.gnu.org/onlinedocs/gcc/Preprocessor-Options.html <https://gcc.gnu.org/onlinedocs/gcc/Preprocessor-Options.html> https://clang.llvm.org/docs/ClangCommandLineReference.html <https://clang.llvm.org/docs/ClangCommandLineReference.html> GTK and WPE ports are using it only in WK2 since r163032. https://trac.webkit.org/changeset/163032 <https://trac.webkit.org/changeset/163032> MSVC has /FI option. /FI (Name Forced Include File) | Microsoft Docs https://docs.microsoft.com/en-us/cpp/build/reference/fi-name-forced-include-... <https://docs.microsoft.com/en-us/cpp/build/reference/fi-name-forced-include-file?view=vs-2017> Unfortunately, it seems that MSVC's precompiled header needs to be included explicitly. /Yu (Use Precompiled Header File) | Microsoft Docs https://docs.microsoft.com/en-us/cpp/build/reference/yu-use-precompiled-head... <https://docs.microsoft.com/en-us/cpp/build/reference/yu-use-precompiled-header-file?view=vs-2017>
There was some work done by aperez to try and enable precompiled headers on all platforms at https://bugs.webkit.org/show_bug.cgi?id=139438 which never landed. I always thought of WebCorePrefix.h as a precompiled headers sort of optimization. Last time I checked though there was definitely an issue with how up to date things were in that file and statement that “the project should be able to build without this header, although we rarely test that.” Is definitely false. From: webkit-dev <webkit-dev-bounces@lists.webkit.org> On Behalf Of Fujii Hironori Sent: Sunday, December 9, 2018 10:35 PM To: Webkit Development List <webkit-dev@lists.webkit.org> Subject: Re: [webkit-dev] WebCorePrefix.h vs. config.h On Sun, Dec 9, 2018 at 8:22 AM Darin Adler <darin@apple.com<mailto:darin@apple.com>> wrote: Best would be to eliminate “config.h”: Change “config.h” into an empty file first, then remove all “config.h” includes, and then remove the file. But to do that, we need to make sure every build system for WebKit supports prefix headers. I don’t know how close to that we are. Maybe close? How can we quickly find out? GCC and Clang support '-include <file>' option. https://gcc.gnu.org/onlinedocs/gcc/Preprocessor-Options.html<https://gcc.gnu.org/onlinedocs/gcc/Preprocessor-Options.html> https://clang.llvm.org/docs/ClangCommandLineReference.html<https://clang.llvm.org/docs/ClangCommandLineReference.html> GTK and WPE ports are using it only in WK2 since r163032. https://trac.webkit.org/changeset/163032<https://trac.webkit.org/changeset/163032> MSVC has /FI option. /FI (Name Forced Include File) | Microsoft Docs https://docs.microsoft.com/en-us/cpp/build/reference/fi-name-forced-include-file?view=vs-2017<https://docs.microsoft.com/en-us/cpp/build/reference/fi-name-forced-include-file?view=vs-2017> Unfortunately, it seems that MSVC's precompiled header needs to be included explicitly. /Yu (Use Precompiled Header File) | Microsoft Docs https://docs.microsoft.com/en-us/cpp/build/reference/yu-use-precompiled-header-file?view=vs-2017<https://docs.microsoft.com/en-us/cpp/build/reference/yu-use-precompiled-header-file?view=vs-2017>
On Dec 9, 2018, at 10:34 PM, Fujii Hironori <fujii.hironori@gmail.com> wrote:
MSVC has /FI option.
/FI (Name Forced Include File) | Microsoft Docs https://docs.microsoft.com/en-us/cpp/build/reference/fi-name-forced-include-...
Unfortunately, it seems that MSVC's precompiled header needs to be included explicitly.
/Yu (Use Precompiled Header File) | Microsoft Docs https://docs.microsoft.com/en-us/cpp/build/reference/yu-use-precompiled-head...
So this seems like the main potential obstacle. We can force an include of a prefix header, or precompile a header, but not both, with the Microsoft compiler. If we deal with this, then I think we could get rid of “config.h”; every other platform can handle a prefix header. Did I understand that right? — Darin
On Wed, Dec 12, 2018 at 7:07 AM Darin Adler <darin@apple.com> wrote:
On Dec 9, 2018, at 10:34 PM, Fujii Hironori <fujii.hironori@gmail.com> wrote:
MSVC has /FI option.
/FI (Name Forced Include File) | Microsoft Docs
https://docs.microsoft.com/en-us/cpp/build/reference/fi-name-forced-include-...
Unfortunately, it seems that MSVC's precompiled header needs to be
included explicitly.
/Yu (Use Precompiled Header File) | Microsoft Docs
https://docs.microsoft.com/en-us/cpp/build/reference/yu-use-precompiled-head...
So this seems like the main potential obstacle. We can force an include of a prefix header, or precompile a header, but not both, with the Microsoft compiler.
It was just my misunderstanding. WebKit Windows ports are already using both /Yu and /FI. https://trac.webkit.org/browser/webkit/trunk/Source/cmake/WebKitMacros.cmake... It seems that WebKit can stop including config.h in all source fils.
participants (5)
-
Alex Christensen
-
Alexey Proskuryakov
-
Darin Adler
-
Don.Olmstead＠sony.com
-
Fujii Hironori