| Summary: | WTF: use Forward.h when appropriate instead of Vector.h | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | JF Bastien <jfbastien> | ||||||||
| Component: | Web Template Framework | Assignee: | JF Bastien <jfbastien> | ||||||||
| Status: | RESOLVED FIXED | ||||||||||
| Severity: | Normal | CC: | achristensen, buildbot, commit-queue, dbates, jfbastien, mjs, mmaxfield, ryanhaddad, saam, webkit-bug-importer | ||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||
| Version: | WebKit Nightly Build | ||||||||||
| Hardware: | Unspecified | ||||||||||
| OS: | Unspecified | ||||||||||
| Bug Depends on: | |||||||||||
| Bug Blocks: | 183725 | ||||||||||
| Attachments: |
|
||||||||||
Created attachment 320871 [details]
patch
As proposed, here's a patch!
It also updates all the locations in our codebase which a quick hack found didn't actually need Vector.h. Is this code churn? Yes! Is it the kind WebKit hates? No! It doesn't make blame any worst, unless you usually blame #include lines and if you do then seriously I suggest finding something else to do. So it's "free" code churn.
Attachment 320871 [details] did not pass style-queue:
ERROR: Source/WebCore/ChangeLog:12: You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible. [changelog/nonewtests] [5]
Total errors found: 1 in 73 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 320871 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=320871&action=review r=me > Source/WebCore/loader/ContentFilter.h:78 > + ContentFilter(Container&&, DocumentLoader&); Is this a valid change to make? (In reply to Saam Barati from comment #3) > Comment on attachment 320871 [details] > patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=320871&action=review > > r=me > > > Source/WebCore/loader/ContentFilter.h:78 > > + ContentFilter(Container&&, DocumentLoader&); > > Is this a valid change to make? Yeah, because: * That function moves the Container argument * It's private, only used by create, which also moves its argument in So from what I can tell someone just forgot to make the parameter an rvalue reference. Created attachment 320901 [details]
patch
Fix for the WPE build, it builds a .cpp file which my local make release doesn't, and that .cpp file expected Vector.h to be included.
Attachment 320901 [details] did not pass style-queue:
ERROR: Source/WebCore/ChangeLog:12: You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible. [changelog/nonewtests] [5]
Total errors found: 1 in 74 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 320901 [details] patch Rejecting attachment 320901 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-02', 'validate-changelog', '--check-oops', '--non-interactive', 320901, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit ChangeLog entry in Source/WebCore/ChangeLog contains OOPS!. Full output: http://webkit-queues.webkit.org/results/4562331 Created attachment 320960 [details]
patch
Fix OOPS.
Comment on attachment 320960 [details] patch Clearing flags on attachment: 320960 Committed r222113: <http://trac.webkit.org/changeset/222113> All reviewed patches have been landed. Closing bug. LayoutTest imported/w3c/web-platform-tests/WebIDL/current-realm.html is hitting an assertion failure on macOS Debug WK2 bots after this change: ASSERTION FAILED: m_workerGlobalScope->hasOneRef() /Volumes/Data/slave/elcapitan-debug/build/Source/WebCore/workers/WorkerThread.cpp(197) : void WebCore::WorkerThread::workerThread() 1 0x1174e6620 WTFCrash 2 0x10bde8c45 WebCore::WorkerThread::workerThread() 3 0x10bde96e8 WebCore::WorkerThread::start()::$_0::operator()() const 4 0x10bde969c WTF::Function<void ()>::CallableWrapper<WebCore::WorkerThread::start()::$_0>::call() 5 0x117523aae WTF::Function<void ()>::operator()() const 6 0x11757286a WTF::Thread::entryPoint(WTF::Thread::NewThreadContext*) 7 0x117578255 WTF::wtfThreadEntryPoint(void*) 8 0x7fff93e8499d _pthread_body 9 0x7fff93e8491a _pthread_body 10 0x7fff93e82351 thread_start LEAK: 1 WebPageProxy https://build.webkit.org/results/Apple%20El%20Capitan%20Debug%20WK2%20(Tests)/r222113%20(3108)/results.html (In reply to Ryan Haddad from comment #11) > LayoutTest imported/w3c/web-platform-tests/WebIDL/current-realm.html is > hitting an assertion failure on macOS Debug WK2 bots after this change: Please disregard, I think this may have been due to another change around the same time. Comment on attachment 320960 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=320960&action=review > Source/WebKit/Scripts/webkit/LegacyMessages-expected.h:-38 > +#include <wtf/Forward.h> > #include <wtf/HashMap.h> > #include <wtf/ThreadSafeRefCounted.h> > -#include <wtf/Vector.h> This change causes a unit test failure in Source/WebKit/Scripts/webkit/messages_unittest.py (see bug #183725). We need to make the messages generator machinery in Source/WebKit/Scripts/webkit/messages.py smarter before we can make this change. Until we fix bug #183724, you must explicitly run the messages generator unit tests. You can do this by running `python Source/WebKit/Scripts/webkit/messages_unittest.py` from a top-level WebKit checkout. I am going to revert the change to this file for now. Feel free to file a bug to make the messages generator code smarter. > Source/WebKit/Scripts/webkit/Messages-expected.h:-38 > +#include <wtf/Forward.h> > #include <wtf/HashMap.h> > #include <wtf/ThreadSafeRefCounted.h> > -#include <wtf/Vector.h> Ditto. |
As discussed on WebKit-dev, I’m moving some code around, and one particular header I have is included everywhere in JSC so I’d like it to be lightweight and include as few other headers as possible. It unfortunately uses WTF::Vector, but it only does so as a pointer: void ohNoes(Vector<Noms>*); It seems like something a forward declaration would fix nicely, and oh joy and wonder we have Forward.h just for this! Here’s what it does: template<typename T, size_t inlineCapacity, typename OverflowHandler, size_t minCapacity, typename Malloc> class Vector; That’s nice and great for T, but all the other template parameters are SOL because Vector is actually declared with default template parameters: template<typename T, size_t inlineCapacity = 0, typename OverflowHandler = CrashOnOverflow, size_t minCapacity = 16, typename Malloc = FastMalloc> class Vector : private VectorBuffer<T, inlineCapacity, Malloc> { The extra painful thing is that, contrary to what I originally thought, one cannot declare Vector in Forward.h and then define it in Vector.h with the same defaults twice! Ideally the compiler would just yell at a mismatch, but instead it says error: template parameter redefines default argument (thanks clang, that’s exactly what I’m trying to do, just tell me if you catch a mismatch and ODR away otherwise). Here’s what I propose: 1. Change the forward declaration in Forward.h to contain the default template parameters (and forward-declare CrashOnOverflow). 2. Remove these default template parameters from Vector.h. 3. Include Forward.h in Vector.h. 4. Optionally, if the WebCritters think it useful, leave a comment just above the Vector definition redirecting to Forward.h for defaults (or more fancy, use size_t inlineCapacity /*=0*/ style like LLVM’s codebase does, and have a tool that checks for consistency). 5. Optionally, try to fix C++20 so this isn’t A Thing anymore. Note that my hopes are low because of modules (why fix it if modules will magically make this not a thing). Alternatively, I could just include Vector.h and be done with it.