[webkit-reviews] review granted: [Bug 198482] JSScript should not keep bytecode cache in memory : [Attachment 371189] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Mon Jun 3 13:40:54 PDT 2019
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.
More information about the webkit-reviews
mailing list