Skip to content

Conversation

@rectified95
Copy link
Contributor

@rectified95 rectified95 commented Apr 13, 2021

Commits: facebook/react-native@cfb5b5e...d04ec4d

Microsoft Reviewers: Open in CodeFlow

@rectified95 rectified95 requested a review from a team as a code owner April 13, 2021 22:39
@NickGerleman NickGerleman changed the title Integrate 1/4 RN nightly build. Integrate April 1 RN nightly build. Apr 13, 2021
@rectified95 rectified95 force-pushed the integrate_apr_1 branch 2 times, most recently from b4c7acb to ee93cae Compare April 14, 2021 22:40
@NickGerleman
Copy link
Contributor

Will look through this today.

Copy link
Contributor

@NickGerleman NickGerleman left a comment

Choose a reason for hiding this comment

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

Skimmed through the upstream commits. LGTM

@rectified95 rectified95 merged commit 1235cab into microsoft:master Apr 15, 2021

MapBufferBuilder::MapBufferBuilder(uint16_t initialSize) {
keyValuesSize_ = initialSize;
keyValues_ = new Byte[keyValuesSize_];
Copy link
Member

Choose a reason for hiding this comment

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

@rectified95 : I know this is from fb, but shouldn't this code be backed by a std::vector just to reduce risk of memory excurions?

Copy link
Contributor Author

@rectified95 rectified95 Apr 15, 2021

Choose a reason for hiding this comment

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

@dannyvv The MapBuffer class has a bunch of potential issues, and the most we can do is submit PRs changing it - otherwise we take it 'as is' from core. (btw what's a memory excursion? There's some custom resize/allocation code in there, if that's what it means.) I think its goal is to be faster than STL.

I'll remove this file once the core PR is merged and we ingest it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants