Skip to content
This repository has been archived by the owner on Apr 22, 2023. It is now read-only.

buffer: make SlowBuffer inherit from Buffer #3228

Closed
wants to merge 1 commit into from
Closed

buffer: make SlowBuffer inherit from Buffer #3228

wants to merge 1 commit into from

Conversation

TooTallNate
Copy link

I wanted people to comment on this one because it's somewhat controversial. To anyone who doesn't like the __proto__ usage, the justification is that we do the same thing to the process object to make it be an EventEmtter.

But basically this frees us from manually having to copy over functions to SlowBuffer's prototype (which has bitten us multiple times in the past). It's also nice for modules like buffertools or my new ref which extend Buffer's prototype, and therefore wouldn't have to do so for SlowBuffer as well.

As an added bonus, the inspect() function is now shared between Buffer and SlowBuffer, removing some duplicate code. There's probably more functions that we could share, but I figured there's probably speed optimizations that should be left in place.

Thoughts?

/cc @isaacs @bnoordhuis @piscisaureus

This frees us from manually having to copy over functions to SlowBuffer's
prototype (which has bitten us multiple times in the past).

As an added bonus, the `inspect()` function is now shared between Buffer
and SlowBuffer, removing some duplicate code.
@isaacs
Copy link

isaacs commented May 7, 2012

I'm ok with this, but would like to hear from @bnoordhuis or @piscisaureus before landing.

@piscisaureus
Copy link

I am okay with it. The only thing is that I always considered SlowBuffer an implementation detail - when do people use that directly?

@TooTallNate
Copy link
Author

@piscisaureus C++ modules returning Buffer instances directly are SlowBuffer instances on the JS-side. I mean you can wrap them into regular Buffer instances with a couple lines of code but there's not much of a point to it.

But exactly because SlowBuffer is an implementation detail, we should abstract the prototype, so that modules extending Buffer don't have to worry about the implementation detail as well.

@bnoordhuis
Copy link
Member

I'm okay with it. @TooTallNate: How sure are you (percentage-wise) that this won't break anything?

@TooTallNate
Copy link
Author

I'm okay with it. @TooTallNate: How sure are you (percentage-wise) that this won't break anything?

@bnoordhuis At least 95% :p. I mean I really can't think of any case where it would break anything. No tests break. Any user-land modules still extending SlowBuffer.prototype would be unaffected. The most controversial part IMO was __proto__ but it doesn't seem like anybody has a problem with that.

@bnoordhuis
Copy link
Member

The most controversial part IMO was proto but it doesn't seem like anybody has a problem with that.

Check the nodejs-dev mailing list. :-)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants