[webkit-reviews] review granted: [Bug 198482] JSScript should not keep bytecode cache in memory : [Attachment 371189] Patch

[bugzilla-daemon at webkit.org]

Saam Barati <sbarati at apple.com> has granted Tadeu Zagallo
<tzagallo at apple.com>'s request for review:
Bug 198482: JSScript should not keep bytecode cache in memory
https://bugs.webkit.org/show_bug.cgi?id=198482

Attachment 371189: Patch

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




--- Comment #3 from Saam Barati <sbarati at apple.com> ---
Comment on attachment 371189
  --> https://bugs.webkit.org/attachment.cgi?id=371189
Patch

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

r=me

> Source/JavaScriptCore/runtime/CachedTypes.cpp:176
> +    Ref<CachedBytecode> releaseMapped(BytecodeCacheError& error)

Kinda weird this returns ref instead of ptr. Why not RefPtr and make nullptr
mean there was an error? I feel like it's weird to construct an empty
CachedBytecode here.

> Source/JavaScriptCore/runtime/CachedTypes.cpp:180
> +	       error.standardError(errno);

nit: This style feels kinda weird. Why not just make error something you assign
to? And by default it's empty? And you can make this a static function that is
a factory constructor. I feel like that's more in line with the style of error
handling I've seen elsewhere in WK. So something like:
```
error = BytecodeCacheError::standardError(errno)
return nullptr 
```

> Source/JavaScriptCore/runtime/CachedTypes.cpp:2411
> +    Encoder encoder(vm, -1);

You could also make -1 the default argument of this function.