Bug 155445 - check-webkit-style: should warn about blank lines after #include "config.h" in TestWebKitAPI
Summary: check-webkit-style: should warn about blank lines after #include "config.h" i...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: David Kilzer (:ddkilzer)
URL:
Keywords:
Depends on: 155394
Blocks:
  Show dependency treegraph
 
Reported: 2016-03-14 10:54 PDT by David Kilzer (:ddkilzer)
Modified: 2016-03-16 15:06 PDT (History)
7 users (show)

See Also:


Attachments
Patch v1 (3.52 KB, patch)
2016-03-14 13:01 PDT, David Kilzer (:ddkilzer)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David Kilzer (:ddkilzer) 2016-03-14 10:54:46 PDT
In Bug 155394, I created a patch (that was auto-committed) that contained a blank line between these two lines in RefLogger.cpp:

#include "config.h"

#include "RefLogger.h"

However, this is against conventional WebKit style, so let's try to flag this in check-webkit-style so it can be spotted earlier and often-er.
Comment 1 David Kilzer (:ddkilzer) 2016-03-14 10:56:58 PDT
For funsies, I wrote a command-line to find all the "violators" in Source:

$ find Source -type f -exec perl -e 'use File::Basename; undef $/; my $file = $ARGV[0]; my $base = basename($file); $base =~ s/\.[a-z]+$//; my $content = <>; print "$file\n" if ($content =~ m/#include "config.h"\n\n#include "${base}.h"/mg);' {} \;
Source/JavaScriptCore/bytecode/UnlinkedCodeBlock.cpp
Source/JavaScriptCore/runtime/CodeCache.cpp
Source/JavaScriptCore/runtime/PropertyDescriptor.cpp
Source/WTF/wtf/dtoa/bignum.cc
Source/WTF/wtf/dtoa/diy-fp.cc
Source/WTF/wtf/dtoa/fast-dtoa.cc
Source/WebCore/bindings/js/JSWorkerGlobalScopeBase.cpp
Source/WebCore/css/CSSBasicShapes.cpp
Source/WebCore/css/CSSGroupingRule.cpp
Source/WebCore/css/WebKitCSSRegionRule.cpp
Source/WebCore/dom/ChildListMutationScope.cpp
Source/WebCore/dom/MutationObserver.cpp
Source/WebCore/dom/MutationObserverInterestGroup.cpp
Source/WebCore/dom/MutationObserverRegistration.cpp
Source/WebCore/fileapi/BlobURL.cpp
Source/WebCore/fileapi/FileReader.cpp
Source/WebCore/fileapi/FileReaderLoader.cpp
Source/WebCore/fileapi/FileReaderSync.cpp
Source/WebCore/html/TimeRanges.cpp
Source/WebCore/html/canvas/CanvasContextAttributes.cpp
Source/WebCore/page/scrolling/ScrollingCoordinator.cpp
Source/WebCore/platform/CrossThreadCopier.cpp
Source/WebCore/platform/FileStream.cpp
Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp
Source/WebCore/platform/graphics/GraphicsLayer.cpp
Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp
Source/WebCore/platform/graphics/ca/LayerFlushScheduler.cpp
Source/WebCore/platform/graphics/ca/win/PlatformCALayerWin.cpp
Source/WebCore/platform/graphics/ca/win/PlatformCALayerWinInternal.cpp
Source/WebCore/platform/graphics/gpu/TilingData.cpp
Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerOwr.cpp
Source/WebCore/platform/graphics/win/MediaPlayerPrivateTaskTimer.cpp
Source/WebCore/platform/network/BlobResourceHandle.cpp
Source/WebCore/platform/network/FormData.cpp
Source/WebCore/platform/win/StructuredExceptionHandlerSuppressor.cpp
Source/WebCore/rendering/FilterEffectRenderer.cpp
Source/WebCore/rendering/RenderLayerBacking.cpp
Source/WebCore/rendering/RenderLayerCompositor.cpp
Source/WebCore/rendering/RenderMarquee.cpp
Source/WebCore/rendering/RenderRuby.cpp
Source/WebCore/rendering/RenderRubyBase.cpp
Source/WebCore/rendering/RenderRubyRun.cpp
Source/WebCore/rendering/RenderRubyText.cpp
Source/WebCore/rendering/style/BasicShapes.cpp
Source/WebCore/svg/SVGZoomEvent.cpp
Source/WebCore/workers/DedicatedWorkerThread.cpp
Source/WebCore/workers/Worker.cpp
Source/WebCore/workers/WorkerScriptLoader.cpp
Source/WebCore/workers/WorkerThread.cpp
Source/WebKit2/NetworkProcess/NetworkLoad.cpp
Source/WebKit2/Shared/WebPopupItem.cpp

And in Tools:

$ find Tools -type f -exec perl -e 'use File::Basename; undef $/; my $file = $ARGV[0]; my $base = basename($file); $base =~ s/\.[a-z]+$//; my $content = <>; print "$file\n" if ($content =~ m/#include "config.h"\n\n#include "${base}.h"/mg);' {} \;
Tools/TestWebKitAPI/PlatformUtilities.cpp
Tools/TestWebKitAPI/Tests/WTF/RefLogger.cpp
Comment 2 David Kilzer (:ddkilzer) 2016-03-14 11:24:04 PDT
(In reply to comment #1)
> And in Tools:
> 
> $ find Tools -type f -exec perl -e 'use File::Basename; undef $/; my $file =
> $ARGV[0]; my $base = basename($file); $base =~ s/\.[a-z]+$//; my $content =
> <>; print "$file\n" if ($content =~ m/#include "config.h"\n\n#include
> "${base}.h"/mg);' {} \;
> Tools/TestWebKitAPI/PlatformUtilities.cpp
> Tools/TestWebKitAPI/Tests/WTF/RefLogger.cpp

Fixed in:

Committed r198141:  <http://trac.webkit.org/r198141>
Comment 3 David Kilzer (:ddkilzer) 2016-03-14 11:25:35 PDT
So we do already have a check for this:

You should not add a blank line before implementation file's own header.  [build/include_order] [4]

Seems like the patch from Bug 155394 somehow missed the check, though.
Comment 4 David Kilzer (:ddkilzer) 2016-03-14 11:43:01 PDT
Strangely, this only appears to fail in RefLogger.cpp:

Steps to reproduce:
1. git checkout bb9b34395effabdaeec2b92e42cee9df70aad271^
2. ./Tools/Scripts/check-webkit-style ./Tools/TestWebKitAPI/Tests/WTF/RefLogger.cpp

Expected results:
ERROR: Tools/TestWebKitAPI/Tests/WTF/RefLogger.cpp:28:  You should not add a blank line before implementation file's own header.  [build/include_order] [4]
Total errors found: 1 in 1 files

Actual results:
Total errors found: 0 in 1 files
Comment 5 David Kilzer (:ddkilzer) 2016-03-14 12:58:38 PDT
So it turns out that Tools/TestWebKitAPI was configured (in Tools/Scripts/webkitpy/style/checker.py, of course) to skip all warnings that start with 'build/include' because at the time this was apparently added, TestWebKitAPI didn't have its own config.h header.

As of r95188 in September 2011, though, Tools/TestWebKitAPI/config.h came into existence, but checker.py was never updated.
Comment 6 David Kilzer (:ddkilzer) 2016-03-14 13:01:48 PDT
Created attachment 274007 [details]
Patch v1
Comment 7 WebKit Commit Bot 2016-03-16 15:06:27 PDT
Comment on attachment 274007 [details]
Patch v1

Clearing flags on attachment: 274007

Committed r198304: <http://trac.webkit.org/changeset/198304>
Comment 8 WebKit Commit Bot 2016-03-16 15:06:31 PDT
All reviewed patches have been landed.  Closing bug.