...
gonna look at this now.
Created attachment 362778 [details] WIP
Created attachment 362785 [details] patch
Comment on attachment 362785 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=362785&action=review Looks good to me. I just have a couple questions that are not directly related to the patch, and the windows build is broken. > Source/JavaScriptCore/runtime/CachedTypes.cpp:221 > + , m_baseAddress(bitwise_cast<const uint8_t*>(baseAddress)) This should probably be `static_cast`, no? > Source/JavaScriptCore/runtime/CachedTypes.cpp:338 > + return bitwise_cast<const uint8_t*>(this) + m_offset; why do you need bitwise_cast here instead of reinterpret_cast? > Source/JavaScriptCore/runtime/CachedTypes.cpp:2164 > + if (m_cacheVersion != JSC_BYTECODE_CACHE_VERSION) > + return false; > + if (m_bootSessionUUID.decode(decoder) != bootSessionUUIDString()) > + return false; Do you think it's worth moving it into a helper method, since it's done here and in decode?
Comment on attachment 362785 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=362785&action=review >> Source/JavaScriptCore/runtime/CachedTypes.cpp:221 >> + , m_baseAddress(bitwise_cast<const uint8_t*>(baseAddress)) > > This should probably be `static_cast`, no? yeah it should be >> Source/JavaScriptCore/runtime/CachedTypes.cpp:338 >> + return bitwise_cast<const uint8_t*>(this) + m_offset; > > why do you need bitwise_cast here instead of reinterpret_cast? I like bitwise_cast because of the static assert it does that the from type should be the same size as the to type. >> Source/JavaScriptCore/runtime/CachedTypes.cpp:2164 >> + return false; > > Do you think it's worth moving it into a helper method, since it's done here and in decode? Yeah I do.
Comment on attachment 362785 [details] patch LGTM
landed in: https://trac.webkit.org/changeset/242239/webkit
<rdar://problem/48492608>
Attempting Windows build fix in: https://trac.webkit.org/changeset/242247/webkit
Hi, I think that patch broke debug builds with gcc: In Source/JavaScriptCore/runtime/CachedTypes.cpp ASSERT_NOT_REACHED (a non-constexpr function) is invoked from the constexpr-function tagFromSourceCodeType. See https://trac.webkit.org/browser/webkit/trunk/Source/JavaScriptCore/runtime/CachedTypes.cpp#L1827
Additional note: With "broken debug build" I meant that compilation is failing on (probably) all architectures (I tested x64 & ARM).
(In reply to Dominik Inführ from comment #11) > Additional note: With "broken debug build" I meant that compilation is > failing on (probably) all architectures (I tested x64 & ARM). Can you land a fix to remove constexpr from it? FWIW, clang debug builds I think are fine
Should we use ASSERT_UNDER_CONSTEXPR_CONTEXT instead?
Nevermind this isn't building as well...
Here is the patch for the build issue: https://bugs.webkit.org/show_bug.cgi?id=195205.
BTW thanks for responding so fast!