-
Notifications
You must be signed in to change notification settings - Fork 29.8k
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.
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
src: use Blob{Des|S}erializer for SEA blobs #47962
Conversation
Review requested:
|
@@ -282,6 +286,14 @@ size_t BlobSerializer<Impl>::WriteString(const std::string& data) { | |||
return written_total; | |||
} | |||
|
|||
template <typename Impl> | |||
size_t BlobSerializer<Impl>::WriteString(const std::string& data) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
size_t BlobSerializer<Impl>::WriteString(const std::string& data) { | |
size_t BlobSerializer<Impl>::WriteString(const std::string_view data) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the method that writes strings (which logs the string directly in debug mode). The one that doesn't is WriteStringView
(which obviously doesn't log that data as string).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that we're using these classes for both snapshots and SEAs, we can update these comments to also mention SEA
:
node/src/blob_serializer_deserializer.h
Lines 9 to 10 in 2e67401
// This is related to the blob that is used in snapshots and has nothing to do | |
// with `node_blob.h`. |
node/src/blob_serializer_deserializer-inl.h
Lines 16 to 17 in 2e67401
// This is related to the blob that is used in snapshots and has nothing to do | |
// with `node_blob.h`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍 (just one typo)
Co-authored-by: Darshan Sen <raisinten@gmail.com>
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
Landed in e2caafa |
I'm getting compiler errors and warnings with GCC 13:
|
@joyeecheung yes it does |
PR-URL: #47962 Reviewed-By: Darshan Sen <raisinten@gmail.com>
PR-URL: nodejs#47962 Reviewed-By: Darshan Sen <raisinten@gmail.com>
PR-URL: nodejs#47962 Reviewed-By: Darshan Sen <raisinten@gmail.com>
No description provided.