[webkit-reviews] review denied: [Bug 192782] [JSC] Cache bytecode to disk : [Attachment 359316] 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 359316: Patch

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




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

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

r- on some more changes. I should have looked closer last time. I'll take
another look once you've addressed my comments.

> Source/JavaScriptCore/runtime/CachedTypes.cpp:82
> +	   m_offset = WTF::roundUpToMultipleOf(sizeof(void*), m_offset);

does this work on 32-bit? isn't alignment wrong for things that need to be
8-byte aligned? It seems like this should take an alignment.

> Source/JavaScriptCore/runtime/CachedTypes.cpp:132
> +    uint8_t* m_buffer;

This should be MallocPtr<uint8_t>.

> Source/JavaScriptCore/runtime/CachedTypes.cpp:231
> +template<typename Source, typename Dst>

Dst seems unused?

> Source/JavaScriptCore/runtime/CachedTypes.cpp:267
> +template<typename Source, typename Dst>

Ditto.

> Source/JavaScriptCore/runtime/CachedTypes.cpp:272
> +    ptrdiff_t offset { s_invalidOffset };

This should be at the bottom of the class per WebKit style.

Also, why is it public? it should probably just have a getter and be prefixed
with m_

> Source/JavaScriptCore/runtime/CachedTypes.cpp:316
> +    bool isEmpty;

Ditto on location/naming/visibility of member.

> Source/JavaScriptCore/runtime/CachedTypes.cpp:329
> +	   T* cachedObject = this->template allocate<T>(encoder);

I think this can just be "allocate<T>(encoder);"

> Source/JavaScriptCore/runtime/CachedTypes.cpp:344
> +	   Source* ptr = operator->()->decode(decoder, args...);

Let's not invoke the operator and make another method get() or something that
the operator-> also calls.

> Source/JavaScriptCore/runtime/CachedTypes.cpp:353
> +	   return this->template buffer<T>();

I think this can just be "return buffer<T>();"

> Source/JavaScriptCore/runtime/CachedTypes.cpp:360
> +	   return this->template buffer<T>();

Ditto.

> Source/JavaScriptCore/runtime/CachedTypes.cpp:366
> +    CachedPtr<T, Source> ptr;

Ditto on naming/visibility/location

> Source/JavaScriptCore/runtime/CachedTypes.cpp:667
> +struct CachedRareData : public CachedObject<UnlinkedCodeBlock::RareData,
CachedRareData> {

Can we name this CachedCodeBlockRaredata? It's not clear which objects rare
data it is right now.