[webkit-reviews] review denied: [Bug 192782] [JSC] Cache bytecode to disk : [Attachment 358679] Patch

[bugzilla-daemon at webkit.org]

Keith Miller <keith_miller at apple.com> has denied Tadeu Zagallo
<tzagallo at apple.com>'s request for review:
Bug 192782: [JSC] Cache bytecode to disk
https://bugs.webkit.org/show_bug.cgi?id=192782

Attachment 358679: Patch

https://bugs.webkit.org/attachment.cgi?id=358679&action=review




--- Comment #9 from Keith Miller <keith_miller at apple.com> ---
Comment on attachment 358679
  --> https://bugs.webkit.org/attachment.cgi?id=358679
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=358679&action=review

I think you need to name things more in line with WebKit style. Overall it
looks pretty good, I'll take a closer look after the style update.

> Source/JavaScriptCore/ChangeLog:14
> +

Can you put a description of how you designed the hierarchy of CachedTypes?
That will probably help people digest this in the future.

> Source/JavaScriptCore/runtime/CachedTypes.cpp:53
> +struct SourceTypeImpl<T, std::enable_if_t<!std::is_fundamental<T>::value &&
!std::is_same<typename T::SourceType_, void>::value>> {

why do you need the std::is_fundamental<T>::value? Wouldn't typename
T::SourceType_ fail for any fundamental type?

> Source/JavaScriptCore/runtime/CachedTypes.cpp:63
> +

You might also want WTF_FORBID_HEAP_ALLOCATION.

> Source/JavaScriptCore/runtime/CachedTypes.cpp:139
> +

Ditto.

> Source/JavaScriptCore/runtime/CachedTypes.cpp:392
> +template<typename T, typename ST = SourceType<T>>

We should use full names for these per WebKit style specifically for things
like Source. The T is probably fine since it's pretty obvious.

> Source/JavaScriptCore/runtime/CachedTypes.cpp:409
> +template<typename T, size_t IC = 0, typename OH = CrashOnOverflow>

Ditto for IC and OH. They should be something like InlineCapacity and
OverflowHandler.

> Source/JavaScriptCore/runtime/CachedTypes.cpp:431
> +template<typename Fst, typename Snd>

Ditto here and elsewhere.

> Source/JavaScriptCore/runtime/CachedTypes.cpp:983
> +	   if (classInfo == SymbolTable::info()) {

Nit: these could all be:

if (auto* symbolTable = jsDynamicCast<SymbolTable*>(cell))
...

> Source/JavaScriptCore/runtime/CachedTypes.cpp:1068
> +	   Vector<uint8_t, 0, UnsafeVectorOverflow> instructionsVector;

Why does this need do be unsafe?