Hi all.
The other day I noticed this in the Code Style Guidelines
(https://webkit.org/code-style-guidelines/#include-statements):
> Header files should never include config.h.
Is there a rationale for it?
config.h is a header file that pulls the compilation flags set in CMake,
some macro functions to query them plus some defaults for specific
platforms (e.g. if platform is Cocoa and WebRTC is enabled,
USE_LIBWEBRTC is defined automatically in WTF/wtf/Platform.h).
As of now, there are lots of instances of header files where macros such
as USE() or ENABLED() are used, yet config.h is not included (even
transitively) by that header file, thus breaking the "include what you
use" rule.
This is not a big deal for the compiler, as every .cpp file in the
project includes config.h before any other header, so by the time those
usages are really compiled those macros will be already defined.
On the other hand, static analysis tools, such as those in IDEs may not
be able to notice this. Unlike compilers, editors and human programmers
often treat files as independent units. For instance, in an editor you
open "LibWebRTCAudioCaptureSource.h", not "LibWebRTCAudioCaptureSource.h
as included by LibWebRTCAudioCaptureSource.cpp in line 28".
Ultimately this can be blamed to C++ fault because includes are a really
sloppy way to require dependencies, but I think we can agree that we
want headers to work as if they were independent units most of the time.
I've seen this issue manifest in QtCreator, as the IDE grays out all the
code inside #if USE() blocks as these macros are not defined or the
expressions inside evaluate to false in the context of that header file.
As an experiment to improve this situation, I first added #pragma once
in config.h files so that they can be included by any header file. Then,
I wrote a Python script that finds all usages of USE() and ENABLED()
macros in header files and adds a #include "config.h" where required.
Objective C headers are ignored by the script using some heuristics.
I built webkitgtk successfully both with and without the patch in gcc
and did not found any noticeable increase in compilation time: the
#pragma once guard is very fast, as expected, so that cannot be ruled as
a reason against. The result is the same but with better static analysis.
As the guidelines are right now I can't get this patch upstream because
they forbid from including config.h... but at least I would like to know
if there a compelling reason for them doing so.
(I attached the patch for the script and a few small edits needed so
that the includes work in all compilation contexts. More edits may be
needed in order for it to build in other platforms, but I could not test
that.)
Regards,
Alicia.
diff --git a/Source/JavaScriptCore/config.h b/Source/JavaScriptCore/config.h
index 1bd9d09741b..2203f266085 100644
--- a/Source/JavaScriptCore/config.h
+++ b/Source/JavaScriptCore/config.h
@@ -19,11 +19,13 @@
*
*/
+#pragma once
+
#if defined(HAVE_CONFIG_H) && HAVE_CONFIG_H && defined(BUILDING_WITH_CMAKE)
#include "cmakeconfig.h"
#endif
-#include "JSExportMacros.h"
+#include "runtime/JSExportMacros.h"
#ifdef __cplusplus
#undef new
diff --git a/Source/WTF/config.h b/Source/WTF/config.h
index ddc4f102190..58eeb534544 100644
--- a/Source/WTF/config.h
+++ b/Source/WTF/config.h
@@ -19,6 +19,8 @@
*
*/
+#pragma once
+
#if defined(HAVE_CONFIG_H) && HAVE_CONFIG_H && defined(BUILDING_WITH_CMAKE)
#include "cmakeconfig.h"
#endif
diff --git a/Source/WTF/wtf/glib/GTypedefs.h b/Source/WTF/wtf/glib/GTypedefs.h
index 51053c03d86..3f0610056b0 100644
--- a/Source/WTF/wtf/glib/GTypedefs.h
+++ b/Source/WTF/wtf/glib/GTypedefs.h
@@ -20,6 +20,11 @@
#ifndef WTF_GTypedefs_h
#define WTF_GTypedefs_h
+/* config.h cannot be included here from all contexts.
+ * For instance, this file is included transitively from ThirdParty/gtest/src/gtest.cc,
+ * which is compiled without any "config.h" in path. */
+#include <wtf/Platform.h>
+
/* Vanilla C code does not seem to be able to handle forward-declaration typedefs. */
#ifdef __cplusplus
diff --git a/Tools/Scripts/include-config-where-needed.py b/Tools/Scripts/include-config-where-needed.py
new file mode 100755
index 00000000000..fa7ed651df0
--- /dev/null
+++ b/Tools/Scripts/include-config-where-needed.py
@@ -0,0 +1,140 @@
+#!/usr/bin/env python
+import os
+import re
+import sys
+
+re_is_guard = re.compile(r'#if.*[^\w](ENABLE|USE)\s*\(')
+re_is_include = re.compile(r'^\s*#\s*include\s*[<"]config.h[>"]\s*')
+re_is_pragma_once = re.compile(r'^\s*#\s*pragma\s+once\s*')
+re_is_ifndef = re.compile(r'^s*#\s*ifndef\s.*')
+re_is_define = re.compile(r'^s*#\s*define\s.*')
+re_is_objc_import = re.compile('^s*#\s*import.*')
+re_is_objc_keyword = re.compile('^s*@\s*(class|interface|property|end)\s.*')
+
+
+def find_pragma_once_line(lines):
+ return next((
+ n_line
+ for n_line, line in enumerate(lines)
+ if re_is_pragma_once.match(line)
+ ), None)
+
+
+def find_traditional_header_guard(lines):
+ ifndef_line = next((
+ n_line
+ for n_line, line in enumerate(lines)
+ if re_is_ifndef.match(line)
+ ), None)
+
+ if ifndef_line is None:
+ return None
+
+ define_line = next((
+ n_line
+ for n_line, line in enumerate(lines)
+ if n_line > ifndef_line and re_is_define.match(line)
+ ), None)
+ return define_line
+
+
+def find_line_to_add_include_config(lines):
+ pragma_once_line = find_pragma_once_line(lines)
+ if pragma_once_line is not None:
+ return pragma_once_line
+ else:
+ return find_traditional_header_guard(lines)
+
+
+def is_objc_header(lines):
+ return any(
+ re_is_objc_import.match(line) or re_is_objc_keyword.match(line)
+ for line in lines
+ )
+
+
+class CouldNotFindWhereToInsertInclude(Exception):
+ def __init__(self, file_path):
+ super(CouldNotFindWhereToInsertInclude, self).__init__(
+ 'Could not find #pragma once or include guard in "%s"' % file_path)
+
+
+def process_file(file_path):
+ with open(file_path, "r") as f:
+ lines = f.read().split("\n")
+
+ if is_objc_header(lines):
+ return
+
+ has_guards = any(
+ re_is_guard.match(line)
+ for line in lines
+ )
+
+ has_include = any(
+ re_is_include.match(line)
+ for line in lines
+ )
+
+ if has_guards and not has_include:
+ # Add the include the line after #pragma once
+ include_after_line = find_line_to_add_include_config(lines)
+ if include_after_line is None:
+ raise CouldNotFindWhereToInsertInclude(file_path)
+
+ lines[include_after_line + 1:include_after_line + 1] = ['', '#include "config.h"']
+
+ # Ensure there is an empty line after the new include in order to
+ # avoid merge conflicts
+ if lines[include_after_line + 3].strip() != "":
+ lines.insert(include_after_line + 3, "")
+
+ # Write the changes
+ with open(file_path, "w") as f:
+ f.write("\n".join(lines))
+
+ return True
+
+def main():
+ base_path = os.path.dirname(__file__) + "/../../Source"
+ modified_files = 0
+ total_files = 0
+ failed_files = []
+
+ ignored_dirs = [
+ "ThirdParty",
+ ]
+
+ ignored_files = [
+ "Platform.h", # USE() and ENABLED() macros are defined here
+ "FeatureDefines.h",
+ "ExportMacros.h",
+ "JSExportMacros.h",
+ "Assertions.h",
+ "GTypedefs.h",
+ ]
+
+ for dir_path, dir_names, file_names in os.walk(base_path):
+ for ignored_dir_name in ignored_dirs:
+ if ignored_dir_name in dir_names:
+ dir_names.remove(ignored_dir_name)
+
+ for file_name in file_names:
+ if file_name.endswith(".h") and file_name not in ignored_files:
+ try:
+ did_modify_file = process_file(os.path.join(dir_path, file_name))
+ if did_modify_file:
+ modified_files += 1
+ except CouldNotFindWhereToInsertInclude:
+ failed_files.append(os.path.join(dir_path, file_name))
+ total_files += 1
+
+ print("Modified %d/%d header files." % (modified_files, total_files))
+
+ if len(failed_files) > 0:
+ print("Failed at %d files because they don't use #pragma once or an include guard:" % len(failed_files))
+ print("\n".join(failed_files))
+
+
+if __name__ == '__main__':
+ main()
_______________________________________________
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev
