Bug 194837 – [bmalloc] DebugHeap::malloc does not have "try" version.

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Yusuke Suzuki]

...

[Yusuke Suzuki]

Created attachment 362468 [details]
Patch

[Mark Lam]

Comment on attachment 362468 [details]
Patch

r=me

[Yusuke Suzuki]

(In reply to Mark Lam from comment #2)
> Comment on attachment 362468 [details]
> Patch
> 
> r=me

Thanks!

[WebKit Commit Bot]

Comment on attachment 362468 [details]
Patch

Clearing flags on attachment: 362468

Committed r241837: <https://trac.webkit.org/changeset/241837>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/48251809>

[Darin Adler]

Why not put the crash on failure at the call sites? Seems strange to pass a boolean just to do a check that the caller could have done.

[Yusuke Suzuki]

(In reply to Darin Adler from comment #7)
> Why not put the crash on failure at the call sites? Seems strange to pass a
> boolean just to do a check that the caller could have done.

The patch adds missing tryAllocate feature into DebugHeap, and this style is aligned to the existing DebugHeap::memalign, bmalloc allocators, and JSC GC Heap Allocators (JavaScriptCore/heap/AllocationFailureMode.h).
I think our (current) typical style is, creating low-level allocator with this style, and constructing tryAllocate / allocate APIs on these low-level allocators, and expose them to clients (e.g. JSArray::tryCreate).
I'm not sure the original intent of this API design. But one pros is that we can gather CRASH into one place instead of putting it at the caller sites, while we still keep good "try" or "not-try" information in stacktrace's function name (`tryAllocate` or `allocate`).
But I don't have strong opinion here as long as the style is consistent :)