Bug 176984 – WTF: use Forward.h when appropriate instead of Vector.h

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Template Framework

[JF Bastien]

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.

[JF Bastien]

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.

[Build Bot]

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.

[Saam Barati]

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?

[JF Bastien]

(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.

[JF Bastien]

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.

[Build Bot]

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.

[WebKit Commit Bot]

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

[JF Bastien]

Created attachment 320960 [details]
patch

Fix OOPS.

[WebKit Commit Bot]

Comment on attachment 320960 [details]
patch

Clearing flags on attachment: 320960

Committed r222113: <http://trac.webkit.org/changeset/222113>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ryan Haddad]

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

[Ryan Haddad]

(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.

[Radar WebKit Bug Importer]

<rdar://problem/34693463>

[Daniel Bates]

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.