buffer: add constants with max buffer and string lengths #13467

addaleax · 2017-06-05T10:02:40Z

This could be useful for programmers to tell whether a value can
be turned into a string or not.

Ref: #13465

/cc @vsemozhetbyt

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

buffer

vsemozhetbyt · 2017-06-05T10:24:20Z

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

bnoordhuis

I infer #13465 is about JSON.stringify() or reading a string from file?

Exporting the maximum string length probably isn't useful for that because you don't normally know upfront how big the string is going to be.

(You might in the read-from-file scenario but even then it's not an exact science: a one-byte string can be twice as big as a two-byte string.)

bnoordhuis · 2017-06-05T11:04:11Z

src/node_buffer.cc

@@ -1271,6 +1271,11 @@ void Initialize(Local<Object> target,
              Integer::NewFromUnsigned(env->isolate(), kMaxLength)).FromJust();

  target->Set(env->context(),
+              FIXED_ONE_BYTE_STRING(env->isolate(), "kMaxStringLength"),


Duplicate of kStringMaxLength. Suggestion: rename that one and update the tests that reference it.

@bnoordhuis Huh, funny I didn’t see that. I used the other one anyway, so I’ve just removed the bits here again.

Fishrock123 · 2017-06-05T12:34:42Z

seems like a good idea

jasnell

I'd much prefer new constants to hang off a constants property to be consistent with fs.constants, os.constants, etc.

If this is a constant intended for end users to use, I'd also much prefer buffer.STRING_MAX_LENGTH. The kWhatever pattern is better suited for internal only things.

addaleax · 2017-06-06T13:00:18Z

@jasnell This is meant to mirror the existing buffer.kMaxLength … I agree, but in this case I’d prefer consistency. I could move kMaxLength as well, if you think that’s a better idea.

jasnell · 2017-06-06T13:45:50Z

I would prefer it, yeah. The existing kMaxLength can be made an alias as we've done with other constants.

Add `buffer.constants`, containing length limits for `Buffer` and `string` instances. This could be useful for programmers to tell whether a value can be turned into a string or not. Ref: nodejs#13465

addaleax · 2017-06-07T18:12:56Z

@jasnell updated, please take a look :)

@vsemozhetbyt @cjihrig you may want to re-review.

cjihrig

Still LGTM

vsemozhetbyt · 2017-06-07T18:44:47Z

I am not sure, but maybe we can add something like the note in the first PR post to the doc, in order to make it clearer why we have .MAX_STRING_LENGTH constant in Buffer. Maybe something about Buffer<->String conversion as one of the use cases.

And maybe we can add some note to the fs doc about sync reading of big files in a string with a connection to this constant.

I am not sure where we can add a note about using JSON stream modules for a [de]serialization of big objects: this is a pretty big source of posted issues, also concerned with this aspect.

addaleax · 2017-06-07T19:26:48Z

CI: https://ci.nodejs.org/job/node-test-commit/10429/

addaleax · 2017-06-16T17:53:37Z

Landed in 1e2905f

Add `buffer.constants`, containing length limits for `Buffer` and `string` instances. This could be useful for programmers to tell whether a value can be turned into a string or not. Ref: #13465 PR-URL: #13467 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de>

This was a typo and accidentally returned the wrong value. Fixes: nodejs#14819 Ref: nodejs#13467

This was a typo and accidentally returned the wrong value. Fixes: #14819 Ref: #13467 PR-URL: #14821 Reviewed-By: Evan Lucas <evanlucas@me.com> Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>

addaleax added buffer Issues and PRs related to the buffer subsystem. semver-minor PRs that contain new features and should be released in the next minor version. v8 engine Issues and PRs related to the V8 dependency. labels Jun 5, 2017

nodejs-github-bot added buffer Issues and PRs related to the buffer subsystem. c++ Issues and PRs that require attention from people who are familiar with C++. labels Jun 5, 2017

vsemozhetbyt approved these changes Jun 5, 2017

View reviewed changes

bnoordhuis reviewed Jun 5, 2017

View reviewed changes

addaleax force-pushed the string-kmaxlength branch from 77a9afa to bbc50f0 Compare June 5, 2017 21:30

cjihrig approved these changes Jun 5, 2017

View reviewed changes

jasnell reviewed Jun 5, 2017

View reviewed changes

addaleax changed the title ~~buffer: expose String::kMaxLength~~ buffer: add constants with max buffer and string lengths Jun 7, 2017

buffer: add constants object

3aced89

Add `buffer.constants`, containing length limits for `Buffer` and `string` instances. This could be useful for programmers to tell whether a value can be turned into a string or not. Ref: nodejs#13465

addaleax force-pushed the string-kmaxlength branch from bbc50f0 to 3aced89 Compare June 7, 2017 18:03

cjihrig approved these changes Jun 7, 2017

View reviewed changes

vsemozhetbyt approved these changes Jun 7, 2017

View reviewed changes

tniessen approved these changes Jun 7, 2017

View reviewed changes

addaleax closed this Jun 16, 2017

addaleax deleted the string-kmaxlength branch June 16, 2017 17:53

addaleax mentioned this pull request Jun 17, 2017

v8.2.0 proposal #13744

Merged

addaleax added a commit to addaleax/node that referenced this pull request Aug 14, 2017

buffer: fix MAX_LENGTH constant export

12ba74e

This was a typo and accidentally returned the wrong value. Fixes: nodejs#14819 Ref: nodejs#13467

addaleax mentioned this pull request Aug 14, 2017

buffer: fix MAX_LENGTH constant export #14821

Closed

3 tasks

targos mentioned this pull request Sep 10, 2017

Error when writing huge of content to a file json #13465

Closed

MylesBorins mentioned this pull request Jan 15, 2018

Semver Minor Tracking nodejs/Release#298

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

buffer: add constants with max buffer and string lengths #13467

buffer: add constants with max buffer and string lengths #13467

addaleax commented Jun 5, 2017

vsemozhetbyt commented Jun 5, 2017

bnoordhuis left a comment

bnoordhuis Jun 5, 2017

addaleax Jun 5, 2017

Fishrock123 commented Jun 5, 2017

jasnell left a comment

addaleax commented Jun 6, 2017

jasnell commented Jun 6, 2017

addaleax commented Jun 7, 2017

cjihrig left a comment

vsemozhetbyt commented Jun 7, 2017 •

edited

Loading

addaleax commented Jun 7, 2017

addaleax commented Jun 16, 2017

buffer: add constants with max buffer and string lengths #13467

buffer: add constants with max buffer and string lengths #13467

Conversation

addaleax commented Jun 5, 2017

Checklist

Affected core subsystem(s)

vsemozhetbyt commented Jun 5, 2017

bnoordhuis left a comment

Choose a reason for hiding this comment

bnoordhuis Jun 5, 2017

Choose a reason for hiding this comment

addaleax Jun 5, 2017

Choose a reason for hiding this comment

Fishrock123 commented Jun 5, 2017

jasnell left a comment

Choose a reason for hiding this comment

addaleax commented Jun 6, 2017

jasnell commented Jun 6, 2017

addaleax commented Jun 7, 2017

cjihrig left a comment

Choose a reason for hiding this comment

vsemozhetbyt commented Jun 7, 2017 • edited Loading

addaleax commented Jun 7, 2017

addaleax commented Jun 16, 2017

vsemozhetbyt commented Jun 7, 2017 •

edited

Loading