Created attachment 382082 [details] Patch

Comment on attachment 382082 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=382082&action=review > Source/JavaScriptCore/ChangeLog:3 > + [GTK][WPE] Fix non-unified builds after r251436 This isn't caused by r251436 > Source/JavaScriptCore/runtime/ObjectInitializationScope.h:32 > +#ifdef NDEBUG > +#include "VM.h" Let's try to avoid protecting #includes with #ifdef NDEBUG, because that would lead to strange debug-only or release-only build failures.

(In reply to Alex Christensen from comment #2) > Comment on attachment 382082 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=382082&action=review > > > Source/JavaScriptCore/ChangeLog:3 > > + [GTK][WPE] Fix non-unified builds after r251436 > > This isn't caused by r251436 Ah, probably I should reword this: my intention here was to mean that the patch addresses all the issues introduced by any commits after non-unified builds were fixed the last time (which is r251436). How about rewording “Fix various non-unified build issues introduced since r251436”? > > Source/JavaScriptCore/runtime/ObjectInitializationScope.h:32 > > +#ifdef NDEBUG > > +#include "VM.h" > > Let's try to avoid protecting #includes with #ifdef NDEBUG, because that > would lead to strange debug-only or release-only build failures. I will remove the guards before landing. Thanks for the review!

Comment on attachment 382082 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=382082&action=review >> Source/JavaScriptCore/runtime/ObjectInitializationScope.h:32 >> +#include "VM.h" > > Let's try to avoid protecting #includes with #ifdef NDEBUG, because that would lead to strange debug-only or release-only build failures. Can you provide some details about why VM.h is needed here? What is the error?

(In reply to Mark Lam from comment #4) > Comment on attachment 382082 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=382082&action=review > > >> Source/JavaScriptCore/runtime/ObjectInitializationScope.h:32 > >> +#include "VM.h" > > > > Let's try to avoid protecting #includes with #ifdef NDEBUG, because that would lead to strange debug-only or release-only build failures. > > Can you provide some details about why VM.h is needed here? What is the > error? The reason I ask is because VM.h is a heavy-weight file, and I'm always wary when we #include it in yet another header file. Just wondering if there are any cheaper solutions for this.

(In reply to Mark Lam from comment #5) > (In reply to Mark Lam from comment #4) > > Comment on attachment 382082 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=382082&action=review > > > > >> Source/JavaScriptCore/runtime/ObjectInitializationScope.h:32 > > >> +#include "VM.h" > > > > > > Let's try to avoid protecting #includes with #ifdef NDEBUG, because that would lead to strange debug-only or release-only build failures. > > > > Can you provide some details about why VM.h is needed here? What is the > > error? > > The reason I ask is because VM.h is a heavy-weight file, and I'm always wary > when we #include it in yet another header file. Just wondering if there are > any cheaper solutions for this. The error is caused from the access to “m_vm” in the ~ObjectInitializationScope() destructor, so I think there is no way around including “VM.h” unless we move the destructor to the .cpp file — probably a bad idea as it is marked with ALWAYS_INLINE (when -DNDEBUG is in use → no debugging → release builds). The compiler error is: In file included from ../Source/JavaScriptCore/runtime/ObjectInitializationScope.cpp:27: ../Source/JavaScriptCore/runtime/ObjectInitializationScope.h:46:13: error: member access into incomplete type 'JSC::VM' m_vm.heap.mutatorFence(); ^ ../Source/JavaScriptCore/runtime/ArrayBuffer.h:42:7: note: forward declaration of 'JSC::VM' class VM; ^ 1 error generated.

(In reply to Adrian Perez from comment #6) > (In reply to Mark Lam from comment #5) > > (In reply to Mark Lam from comment #4) > > > Comment on attachment 382082 [details] > > > Patch > > > > > > View in context: > > > https://bugs.webkit.org/attachment.cgi?id=382082&action=review > > > > > > >> Source/JavaScriptCore/runtime/ObjectInitializationScope.h:32 > > > >> +#include "VM.h" > > > > > > > > Let's try to avoid protecting #includes with #ifdef NDEBUG, because that would lead to strange debug-only or release-only build failures. > > > > > > Can you provide some details about why VM.h is needed here? What is the > > > error? > > > > The reason I ask is because VM.h is a heavy-weight file, and I'm always wary > > when we #include it in yet another header file. Just wondering if there are > > any cheaper solutions for this. > > The error is caused from the access to “m_vm” in the > ~ObjectInitializationScope() > destructor, so I think there is no way around including “VM.h” unless we move > the destructor to the .cpp file — probably a bad idea as it is marked with > ALWAYS_INLINE (when -DNDEBUG is in use → no debugging → release builds). > > The compiler error is: > > In file included from > ../Source/JavaScriptCore/runtime/ObjectInitializationScope.cpp:27: > ../Source/JavaScriptCore/runtime/ObjectInitializationScope.h:46:13: error: > member access into incomplete type 'JSC::VM' > m_vm.heap.mutatorFence(); > ^ > ../Source/JavaScriptCore/runtime/ArrayBuffer.h:42:7: note: forward > declaration of 'JSC::VM' > class VM; > ^ > 1 error generated. Thanks. Go ahead and #include it for now. We can look at an alternative solution later if needed.

Created attachment 382141 [details] Patch for landing

The commit-queue encountered the following flaky tests while processing attachment 382141 [details]: media/modern-media-controls/compact-media-controls/compact-media-controls-constructor.html bug 193587 (author: graouts@apple.com) The commit-queue is continuing to process your patch.

Comment on attachment 382141 [details] Patch for landing Clearing flags on attachment: 382141 Committed r251690: <https://trac.webkit.org/changeset/251690>

All reviewed patches have been landed. Closing bug.

<rdar://problem/56692913>