Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

src: move BlobSerializerDeserializer to a separate header file #47933

Closed
wants to merge 3 commits into from
Closed

src: move BlobSerializerDeserializer to a separate header file #47933

wants to merge 3 commits into from

Conversation

RaisinTen
Copy link
Contributor

This should make it possible to reuse the BlobSerializer and the
BlobDeserializer classes in SEAs to generate and parse the injected
blob.

This change also resolves this TODO:

node/src/node_snapshotable.cc

Line 187 in 4f69aae

// TODO(joyeecheung): move it to the separate header file.

Refs: #47458
Signed-off-by: Darshan Sen [email protected]

cc @joyeecheung

@RaisinTen
src: move BlobSerializerDeserializer to a separate header file
0bdbb85 
This should make it possible to reuse the BlobSerializer and the
BlobDeserializer classes in SEAs to generate and parse the injected
blob.

This change also resolves this TODO:
https://github.com/nodejs/node/blob/4f69aae6a04a460f267005dcf6551959064b3238/src/node_snapshotable.cc#L187

Refs: nodejs#47458
Signed-off-by: Darshan Sen <[email protected]>
@RaisinTen
src: split into a blob_serializer_deserializer-inl.h file
22e2b79 
To keep the main header file readable and clean.

Refs: https://github.com/nodejs/node/blob/4f69aae6a04a460f267005dcf6551959064b3238/src/README.md?plain=1#L52-L62
Signed-off-by: Darshan Sen <[email protected]>
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/gyp
  • @nodejs/startup

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels May 9, 2023
@RaisinTen RaisinTen added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels May 9, 2023
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@RaisinTen RaisinTen added the commit-queue Add this label to land a pull request using GitHub Actions. label May 9, 2023
@github-actions github-actions bot mentioned this pull request May 10, 2023
Open
25 tasks
@RaisinTen RaisinTen removed the commit-queue Add this label to land a pull request using GitHub Actions. label May 10, 2023
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/51735/

@github-actions github-actions bot mentioned this pull request May 11, 2023
Open
24 tasks
RaisinTen added a commit that referenced this pull request May 11, 2023
@RaisinTen
src: move BlobSerializerDeserializer to a separate header file
78fe139 
This should make it possible to reuse the BlobSerializer and the
BlobDeserializer classes in SEAs to generate and parse the injected
blob.

This change also resolves this TODO:
https://github.com/nodejs/node/blob/4f69aae6a04a460f267005dcf6551959064b3238/src/node_snapshotable.cc#L187

Refs: #47458
Signed-off-by: Darshan Sen <[email protected]>
PR-URL: #47933
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@RaisinTen
Copy link
Contributor Author

Landed in 78fe139

@RaisinTen RaisinTen closed this May 11, 2023
@RaisinTen RaisinTen deleted the move-BlobSerializerDeserializer-to-separate-header branch May 11, 2023 12:50
@github-actions github-actions bot mentioned this pull request May 12, 2023
Open
23 tasks
targos pushed a commit that referenced this pull request May 12, 2023
@RaisinTen @targos
src: move BlobSerializerDeserializer to a separate header file
0c06bfd 
This should make it possible to reuse the BlobSerializer and the
BlobDeserializer classes in SEAs to generate and parse the injected
blob.

This change also resolves this TODO:
https://github.com/nodejs/node/blob/4f69aae6a04a460f267005dcf6551959064b3238/src/node_snapshotable.cc#L187

Refs: #47458
Signed-off-by: Darshan Sen <[email protected]>
PR-URL: #47933
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
This was referenced May 13, 2023
Open
Open
Open
@targos targos mentioned this pull request May 15, 2023
Merged
@github-actions github-actions bot mentioned this pull request May 16, 2023
Open
19 tasks
danielleadams pushed a commit that referenced this pull request Jul 6, 2023
@RaisinTen @danielleadams
src: move BlobSerializerDeserializer to a separate header file
538520c 
This should make it possible to reuse the BlobSerializer and the
BlobDeserializer classes in SEAs to generate and parse the injected
blob.

This change also resolves this TODO:
https://github.com/nodejs/node/blob/4f69aae6a04a460f267005dcf6551959064b3238/src/node_snapshotable.cc#L187

Refs: #47458
Signed-off-by: Darshan Sen <[email protected]>
PR-URL: #47933
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
MoLow pushed a commit to MoLow/node that referenced this pull request Jul 6, 2023
@RaisinTen @MoLow
src: move BlobSerializerDeserializer to a separate header file
4c55681 
This should make it possible to reuse the BlobSerializer and the
BlobDeserializer classes in SEAs to generate and parse the injected
blob.

This change also resolves this TODO:
https://github.com/nodejs/node/blob/4f69aae6a04a460f267005dcf6551959064b3238/src/node_snapshotable.cc#L187

Refs: nodejs#47458
Signed-off-by: Darshan Sen <[email protected]>
PR-URL: nodejs#47933
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@danielleadams danielleadams mentioned this pull request Jul 10, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jasnell jasnell jasnell left review comments

@joyeecheung joyeecheung joyeecheung approved these changes

@cjihrig cjihrig cjihrig approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@RaisinTen @nodejs-github-bot @jasnell @cjihrig @joyeecheung