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

smalloc: allow different external array types#5953

Merged
trevnorris merged 2 commits intonodejs:masterfrom
trevnorris:alloc-all-the-types
Aug 7, 2013
Merged

smalloc: allow different external array types#5953
trevnorris merged 2 commits intonodejs:masterfrom
trevnorris:alloc-all-the-types

Conversation

@trevnorris
Copy link

Users can specify what type of external array they want using
smalloc.TYPES.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/enum/Enum/ and s/Include/Includes/ (or maybe just "Contains" or "Has the following members".)

@bnoordhuis
Copy link
Member

LGTM, no comments.

@trevnorris
Copy link
Author

I still need to add a few tests. Also getting the following from cpplint:

src/smalloc.cc:87:  Using sizeof(type).  Use sizeof(varname) instead if possible  [runtime/sizeof] [1]
src/smalloc.cc:89:  Using sizeof(type).  Use sizeof(varname) instead if possible  [runtime/sizeof] [1]

@indutny anyway we can fix this? the function doesn't have a varname available.

@isaacs think I have a use case for this in core, and fwiw it causes zero performance regression.

@isaacs
Copy link

isaacs commented Aug 5, 2013

My only objection is feature creep, but this is pretty reasonable. If there's a use for it (either core or userland) then lgtm.

@trevnorris
Copy link
Author

I have half a dozen uses for user land, but I hear you on feature creep. I
thought it would be useful for a couple things internally. One example like
to more easily keep track of hot parameters like the next tick infoBox
instead of setting up the struct and all that. Still TBD whether that will
work.

If I can't find a proper use in core then I'll put it on the back burner
unless the community mentions a need.

@trevnorris
Copy link
Author

@bnoordhuis Made a few additional changes since you're last review. Also squashed everything. Mind giving it one more look over?

src/smalloc.cc Outdated
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unsigned long?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... yeah. fixed.

@bnoordhuis
Copy link
Member

LGTM

* Moved the ToObject check out of smalloc::Alloc and into JS. Direct
  usage of that method is for internal use only and so can bypass the
  possible coercion.
* Same has been done with smalloc::SliceOnto.
* smalloc::CopyOnto will now throw if passed argument is not an object.
* Remove extra TargetFreeCallback function. There was a use for it when
  it was working with a Local<T>, but that code has been removed making
  the function superfluous.
smalloc.alloc now accepts an optional third argument which allows
specifying the type of array that should be allocated. All available
types are now located on smalloc.Types.
@trevnorris trevnorris merged commit cec8159 into nodejs:master Aug 7, 2013
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants