Improve header guard checks in check-webkit-style. Bug 206448 demonstrated that the header guard checks in check-webkit-style would not warn about new header files that were missing "#pragma once" statements because it assumed that any header file that didn't already have an #ifndef/#define/#endif check _might_ be an Objective-C header that didn't need a header guard. We can do better than this.
Created attachment 388171 [details] Patch v1
Here's how to find header files with missing guards: $ ./Tools/Scripts/check-webkit-style --filter=-,+build/header_guard_missing [path] To find "legacy header guards" that could be updated to "#pragma once", use: $ ./Tools/Scripts/check-webkit-style --filter=-,+build/header_guard [path]
Hmm...apparently this patch is not python3-compliant: [866/1893] webkitpy.style.checkers.cpp_unittest.CppStyleTest.test_build_header_guard erred: Traceback (most recent call last): File "/Volumes/Data/worker/WebKitPy-Tests-EWS/build/Tools/Scripts/webkitpy/style/checkers/cpp_unittest.py", line 2684, in test_build_header_guard self.assert_header_guard('#pragma once', '') File "/Volumes/Data/worker/WebKitPy-Tests-EWS/build/Tools/Scripts/webkitpy/style/checkers/cpp_unittest.py", line 348, in assert_header_guard self.perform_header_guard_check(code, filename)) File "/Volumes/Data/worker/WebKitPy-Tests-EWS/build/Tools/Scripts/webkitpy/style/checkers/cpp_unittest.py", line 314, in perform_header_guard_check return self.perform_lint(code, filename, basic_error_rules) File "/Volumes/Data/worker/WebKitPy-Tests-EWS/build/Tools/Scripts/webkitpy/style/checkers/cpp_unittest.py", line 260, in perform_lint self.process_file_data(filename, extension, lines, error_collector, unit_test_config) File "/Volumes/Data/worker/WebKitPy-Tests-EWS/build/Tools/Scripts/webkitpy/style/checkers/cpp_unittest.py", line 254, in process_file_data checker.check(lines) File "/Volumes/Data/worker/WebKitPy-Tests-EWS/build/Tools/Scripts/webkitpy/style/checkers/cpp.py", line 4279, in check self.handle_style_error, self.min_confidence) File "/Volumes/Data/worker/WebKitPy-Tests-EWS/build/Tools/Scripts/webkitpy/style/checkers/cpp.py", line 4111, in _process_lines check_for_header_guard(filename, lines, error) File "/Volumes/Data/worker/WebKitPy-Tests-EWS/build/Tools/Scripts/webkitpy/style/checkers/cpp.py", line 963, in check_for_header_guard if reduce(lambda x, y: x or y, map(lambda x: x in line, ['@class', '@interface', '@prototype'])): NameError: name 'reduce' is not defined
Created attachment 388173 [details] Patch v2
Created attachment 388178 [details] Patch v3
See Bug 206481 for output from this script for bmalloc, WTF, JavaScriptCore. Below are WebCore, WebKitLegacy and WebKit (so you may evaluate possible false positive rate when reviewing): $ ./Tools/Scripts/check-webkit-style --filter=-,+build/header_guard_missing Source/WebCore ERROR: Source/WebCore/bridge/npruntime_internal.h:28: Missing #pragma once for header guard. [build/header_guard_missing] [5] ERROR: Source/WebCore/platform/network/HTTPStatusCodes.h:4: Missing #pragma once for header guard. [build/header_guard_missing] [5] ERROR: Source/WebCore/platform/network/mac/WebCoreURLResponse.h:29: Missing #pragma once for header guard. [build/header_guard_missing] [5] ERROR: Source/WebCore/platform/ios/wak/WebCoreThreadSystemInterface.h:26: Missing #pragma once for header guard. [build/header_guard_missing] [5] ERROR: Source/WebCore/platform/mac/WebNSAttributedStringExtras.h:26: Missing #pragma once for header guard. [build/header_guard_missing] [5] ERROR: Source/WebCore/platform/cocoa/SystemVersion.h:25: Missing #pragma once for header guard. [build/header_guard_missing] [5] ERROR: Source/WebCore/platform/mediastream/libwebrtc/VideoToolBoxDecoderFactory.h:2: Missing #pragma once for header guard. [build/header_guard_missing] [5] ERROR: Source/WebCore/platform/mediastream/libwebrtc/VideoToolBoxEncoderFactory.h:2: Missing #pragma once for header guard. [build/header_guard_missing] [5] ERROR: Source/WebCore/platform/graphics/FormatConverter.h:28: Missing #pragma once for header guard. [build/header_guard_missing] [5] ERROR: Source/WebCore/platform/graphics/ImageBufferData.h:26: Missing #pragma once for header guard. [build/header_guard_missing] [5] ERROR: Source/WebCore/platform/graphics/avfoundation/cf/AVFoundationCFSoftLinking.h:26: Missing #pragma once for header guard. [build/header_guard_missing] [5] ERROR: Source/WebCore/editing/cocoa/AutofillElements.h:26: Missing #pragma once for header guard. [build/header_guard_missing] [5] ERROR: Source/WebCore/PAL/pal/ios/QuickLookSoftLink.h:25: Missing #pragma once for header guard. [build/header_guard_missing] [5] ERROR: Source/WebCore/PAL/pal/spi/ios/SQLite3SPI.h:26: Missing #pragma once for header guard. [build/header_guard_missing] [5] ERROR: Source/WebCore/bindings/js/StructuredClone.h:27: Missing #pragma once for header guard. [build/header_guard_missing] [5] Total errors found: 15 in 9934 files $ ./Tools/Scripts/check-webkit-style --filter=-,+build/header_guard_missing Source/WebKitLegacy ERROR: Source/WebKitLegacy/WebCoreSupport/WebViewGroup.h:26: Missing #pragma once for header guard. [build/header_guard_missing] [5] ERROR: Source/WebKitLegacy/win/WebKitStatisticsPrivate.h:29: Missing #pragma once for header guard. [build/header_guard_missing] [5] ERROR: Source/WebKitLegacy/win/resource.h:33: Missing #pragma once for header guard. [build/header_guard_missing] [5] ERROR: Source/WebKitLegacy/win/WebDocumentLoader.h:26: Missing #pragma once for header guard. [build/header_guard_missing] [5] ERROR: Source/WebKitLegacy/win/WebPreferenceKeysPrivate.h:29: Missing #pragma once for header guard. [build/header_guard_missing] [5] ERROR: Source/WebKitLegacy/win/WebCoreSupport/WebContextMenuClient.h:26: Missing #pragma once for header guard. [build/header_guard_missing] [5] ERROR: Source/WebKitLegacy/win/WebCoreSupport/WebDesktopNotificationsDelegate.h:31: Missing #pragma once for header guard. [build/header_guard_missing] [5] ERROR: Source/WebKitLegacy/win/WebCoreSupport/WebChromeClient.h:27: Missing #pragma once for header guard. [build/header_guard_missing] [5] ERROR: Source/WebKitLegacy/win/WebCoreSupport/WebDragClient.h:29: Missing #pragma once for header guard. [build/header_guard_missing] [5] ERROR: Source/WebKitLegacy/mac/Misc/WebKitStatisticsPrivate.h:29: Missing #pragma once for header guard. [build/header_guard_missing] [5] ERROR: Source/WebKitLegacy/mac/Misc/WebTypesInternal.h:29: Missing #pragma once for header guard. [build/header_guard_missing] [5] ERROR: Source/WebKitLegacy/mac/Misc/WebLocalizableStrings.h:29: Missing #pragma once for header guard. [build/header_guard_missing] [5] ERROR: Source/WebKitLegacy/mac/DOM/WebAutocapitalizeTypes.h:26: Missing #pragma once for header guard. [build/header_guard_missing] [5] ERROR: Source/WebKitLegacy/mac/WebView/WebPreferenceKeysPrivate.h:29: Missing #pragma once for header guard. [build/header_guard_missing] [5] ERROR: Source/WebKitLegacy/mac/WebView/WebMediaPlaybackTargetPicker.h:26: Missing #pragma once for header guard. [build/header_guard_missing] [5] Total errors found: 15 in 1178 files $ ./Tools/Scripts/check-webkit-style --filter=-,+build/header_guard_missing Source/WebKit 2>&1 | grep -v Skipping ERROR: Source/WebKit/UIProcess/API/C/WKPageRenderingProgressEventsInternal.h:26: Missing #pragma once for header guard. [build/header_guard_missing] [5] ERROR: Source/WebKit/UIProcess/API/Cocoa/WKPreferencesPrivate.h:2: Missing #pragma once for header guard. [build/header_guard_missing] [5] ERROR: Source/WebKit/UIProcess/API/Cocoa/WKWebArchive.h:26: Missing #pragma once for header guard. [build/header_guard_missing] [5] ERROR: Source/WebKit/UIProcess/API/Cocoa/WKNavigationData.h:26: Missing #pragma once for header guard. [build/header_guard_missing] [5] ERROR: Source/WebKit/Platform/IPC/Attachment.h:27: Missing #pragma once for header guard. [build/header_guard_missing] [5] ERROR: Source/WebKit/Platform/ios/AccessibilityIOS.h:26: Missing #pragma once for header guard. [build/header_guard_missing] [5] ERROR: Source/WebKit/Shared/ShareSheetCallbackID.h:26: Missing #pragma once for header guard. [build/header_guard_missing] [5] ERROR: Source/WebKit/Shared/mac/SecItemShim.h:26: Missing #pragma once for header guard. [build/header_guard_missing] [5] ERROR: Source/WebKit/NetworkProcess/glib/WebKitCachedResolver.h:26: Missing #pragma once for header guard. [build/header_guard_missing] [5] Total errors found: 9 in 3779 files
Comment on attachment 388178 [details] Patch v3 View in context: https://bugs.webkit.org/attachment.cgi?id=388178&action=review > Tools/Scripts/webkitpy/style/checkers/cpp.py:965 > + if functools.reduce(lambda x, y: x or y, map(lambda x: x in line, ['@class', '@interface', '@protocol'])): > + has_objc_keywords = True A header with an Objective-C keyword is not necessarily an Objective-C-only header. There could be an #ifdef __OBJC__ guarding the Objective-C part.
Comment on attachment 388178 [details] Patch v3 View in context: https://bugs.webkit.org/attachment.cgi?id=388178&action=review >> Tools/Scripts/webkitpy/style/checkers/cpp.py:965 >> + has_objc_keywords = True > > A header with an Objective-C keyword is not necessarily an Objective-C-only header. There could be an #ifdef __OBJC__ guarding the Objective-C part. Yep, see the `has_objc_check` check three lines above, and the logic for returning early two lines below. I felt Comment #6 was a pretty good indication of a low false-positive rate.
Comment on attachment 388178 [details] Patch v3 View in context: https://bugs.webkit.org/attachment.cgi?id=388178&action=review >>> Tools/Scripts/webkitpy/style/checkers/cpp.py:965 >>> + has_objc_keywords = True >> >> A header with an Objective-C keyword is not necessarily an Objective-C-only header. There could be an #ifdef __OBJC__ guarding the Objective-C part. > > Yep, see the `has_objc_check` check three lines above, and the logic for returning early two lines below. > > I felt Comment #6 was a pretty good indication of a low false-positive rate. For example, looking at Source/WebCore/platform/cocoa/SystemVersion.h (which was flagged as missing a "#pragma once" statement in Comment #6) without looking at where it's included, you might guess that it's an Objective-C++ header, but maybe it's relying on an `OBJC_CLASS NSString;` definition from another header in UnifiedSources. Since it's currently ambiguous, I'd probably add an "@class NSString;" statement to the header to make it obvious that it's only used by Objective-C[++] sources.
Comment on attachment 388178 [details] Patch v3 View in context: https://bugs.webkit.org/attachment.cgi?id=388178&action=review >>>> Tools/Scripts/webkitpy/style/checkers/cpp.py:965 >>>> + has_objc_keywords = True >>> >>> A header with an Objective-C keyword is not necessarily an Objective-C-only header. There could be an #ifdef __OBJC__ guarding the Objective-C part. >> >> Yep, see the `has_objc_check` check three lines above, and the logic for returning early two lines below. >> >> I felt Comment #6 was a pretty good indication of a low false-positive rate. > > For example, looking at Source/WebCore/platform/cocoa/SystemVersion.h (which was flagged as missing a "#pragma once" statement in Comment #6) without looking at where it's included, you might guess that it's an Objective-C++ header, but maybe it's relying on an `OBJC_CLASS NSString;` definition from another header in UnifiedSources. > > Since it's currently ambiguous, I'd probably add an "@class NSString;" statement to the header to make it obvious that it's only used by Objective-C[++] sources. Here's an example of a true positive from Comment #6 with an __OBJC__ check where the "#pragma once" statement is missing: Source/WebCore/platform/network/mac/WebCoreURLResponse.h
Comment on attachment 388178 [details] Patch v3 View in context: https://bugs.webkit.org/attachment.cgi?id=388178&action=review >>>>> Tools/Scripts/webkitpy/style/checkers/cpp.py:965 >>>>> + has_objc_keywords = True >>>> >>>> A header with an Objective-C keyword is not necessarily an Objective-C-only header. There could be an #ifdef __OBJC__ guarding the Objective-C part. >>> >>> Yep, see the `has_objc_check` check three lines above, and the logic for returning early two lines below. >>> >>> I felt Comment #6 was a pretty good indication of a low false-positive rate. >> >> For example, looking at Source/WebCore/platform/cocoa/SystemVersion.h (which was flagged as missing a "#pragma once" statement in Comment #6) without looking at where it's included, you might guess that it's an Objective-C++ header, but maybe it's relying on an `OBJC_CLASS NSString;` definition from another header in UnifiedSources. >> >> Since it's currently ambiguous, I'd probably add an "@class NSString;" statement to the header to make it obvious that it's only used by Objective-C[++] sources. > > Here's an example of a true positive from Comment #6 with an __OBJC__ check where the "#pragma once" statement is missing: > > Source/WebCore/platform/network/mac/WebCoreURLResponse.h I see. Had missed the other checks.
Comment on attachment 388178 [details] Patch v3 Clearing flags on attachment: 388178 Committed r254831: <https://trac.webkit.org/changeset/254831>
All reviewed patches have been landed. Closing bug.
<rdar://problem/58740920>