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

fs: write strings directly to disk#5747

Merged
trevnorris merged 2 commits intonodejs:masterfrom
trevnorris:fs-no-buffer-first
Jul 30, 2013
Merged

fs: write strings directly to disk#5747
trevnorris merged 2 commits intonodejs:masterfrom
trevnorris:fs-no-buffer-first

Conversation

@trevnorris
Copy link

Instead of first converting the string to a Buffer instance, use the string data directly to write to disk.

This partially addresses #5607

Copy link
Member

Choose a reason for hiding this comment

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

Too jargon-y perhaps?

Copy link
Author

Choose a reason for hiding this comment

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

Good call. Simplified.

@isaacs
Copy link

isaacs commented Jul 19, 2013

lgtm, but @bnoordhuis or @tjfontaine or @indutny should weigh in on the C++ bits as well.

src/node_file.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.

must_free_

@bnoordhuis
Copy link
Member

LGTM save for the uv_fs_write() thing but that's contingent on a new libuv release.

@trevnorris
Copy link
Author

@bnoordhuis thanks, I'll wait for the next libuv version to land before landing this.

The method is useful elsewhere when needing to check if external and
grab data.
Prior, strings would first be converted to a Buffer before being written
to disk. Now the intermediary step has been removed.

Other changes of note:

* Class member "must_free" was added to req_wrap so to track if the
  memory needs to be manually cleaned up after use.
* External String Resource support, so the memory will be used directly
  instead of copying out the data.
* Docs have been updated to reflect that if position is not a number
  then it will assume null. Previously it specified the argument must be
  null, but that was not how the code worked. An attempt was made to
  only support == null, but there were too many tests that assumed !=
  number would be enough.
* Docs update show some of the write/writeSync arguments are optional.
@trevnorris trevnorris merged commit 7ca77ea into nodejs:master Jul 30, 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