-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Fix build on 32-bit #51010
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix build on 32-bit #51010
Conversation
EWS run on previous version of this PR (hash 5238e23) |
Source/WebCore/dom/EventTarget.h
Outdated
#if !USE(SYSTEM_MALLOC) | ||
#include <bmalloc/TZoneHeap.h> | ||
#include <bmalloc/bmalloc.h> | ||
#endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This does not look right. Do we need to include bmalloc headers?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, 'bmalloc.h' was not adtually needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this means that we should include wtf/TZoneMalloc instead
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why isn't it wtf/TZoneMalloc.h? Also, SYSTEM_MALLOC guard is not necessary
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Header wtf/TZoneMalloc.h
is already included in the file. It doesn't seem header <bmalloc/TZoneHeap.h>
is necessary as it's already included via wtf/TZoneMalloc.h
, so I removed it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW, the inclusion of header bmalloc/TZoneHeap.h
was causing a build error in WebKitGTK 32-bit (Debian 12):
/home/buildbot/worker/webkit/WebKitBuild/GTK/Release/WebCore/PrivateHeaders/WebCore/EventTarget.h:50:10: fatal error: bmalloc/TZoneHeap.h: No such file or directory
50 | #include <bmalloc/TZoneHeap.h>
| ^~~~~~~~~~~~~~~~~~~~~
To fix this build error I followed the same pattern as other files that also include <bmalloc/TZoneHeap.h>
, such as https://github.com/WebKit/WebKit/blob/main/Source/WebKit/Platform/IPC/Connection.h#L87 and https://github.com/WebKit/WebKit/blob/main/Source/WebKit/Platform/IPC/Decoder.h#L48.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please include wtf headers
5238e23
to
d981418
Compare
EWS run on previous version of this PR (hash d981418) |
Sorry, I don't understand this comment. The changes applied are enough to make WebKitGTK build on Debian 12 32-bit. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See Justin's comment. Including bmalloc header is not correct
d981418
to
51daa0c
Compare
EWS run on previous version of this PR (hash 51daa0c) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove NO TEST comment in the commit log.
51daa0c
to
22ab395
Compare
EWS run on current version of this PR (hash 22ab395) |
Reviewed by Yusuke Suzuki and Justin Michaud. I attempted to build WebKitGTK on Debian 12 32-bit using only system libraries ('--no-experimetal-features'). Exactly the same build procedure as the WebKitGTK Debian 12 64-bit bot, but on 32-bit. I got a couple of build errors, which this commit fixes. * Source/JavaScriptCore/CMakeLists.txt: * Source/WebCore/dom/EventTarget.h: Canonical link: https://commits.webkit.org/300313@main
22ab395
to
f74166b
Compare
Committed 300313@main (f74166b): https://commits.webkit.org/300313@main Reviewed commits have been landed. Closing PR #51010 and removing active labels. |
f74166b
22ab395