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.

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

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

Closed
wants to merge 1 commit into from

Conversation

addaleax
Copy link
Member

@addaleax addaleax commented Jun 5, 2017

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

@addaleax 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 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
Copy link
Contributor

Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

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.)

@@ -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"),
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

@Fishrock123
Copy link
Contributor

seems like a good idea

Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

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
Copy link
Member Author

addaleax commented Jun 6, 2017

@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
Copy link
Member

jasnell commented Jun 6, 2017

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

@addaleax addaleax changed the title buffer: expose String::kMaxLength buffer: add constants with max buffer and string lengths Jun 7, 2017
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
Copy link
Member Author

addaleax commented Jun 7, 2017

@jasnell updated, please take a look :)

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

Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

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

Still LGTM

@vsemozhetbyt
Copy link
Contributor

vsemozhetbyt commented Jun 7, 2017

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
Copy link
Member Author

addaleax commented Jun 7, 2017

@addaleax
Copy link
Member Author

Landed in 1e2905f

@addaleax addaleax closed this Jun 16, 2017
@addaleax addaleax deleted the string-kmaxlength branch June 16, 2017 17:53
addaleax added a commit that referenced this pull request Jun 16, 2017
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 <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
addaleax added a commit that referenced this pull request Jun 17, 2017
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 <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
@addaleax addaleax mentioned this pull request Jun 17, 2017
addaleax added a commit that referenced this pull request Jun 21, 2017
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 <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
addaleax added a commit that referenced this pull request Jun 24, 2017
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 <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
rvagg pushed a commit that referenced this pull request Jun 29, 2017
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 <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
addaleax added a commit that referenced this pull request Jul 11, 2017
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 <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
addaleax added a commit to addaleax/node that referenced this pull request Aug 14, 2017
This was a typo and accidentally returned the wrong value.

Fixes: nodejs#14819
Ref: nodejs#13467
addaleax added a commit that referenced this pull request Aug 17, 2017
This was a typo and accidentally returned the wrong value.

Fixes: #14819
Ref: #13467
PR-URL: #14821
Reviewed-By: Evan Lucas <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
MylesBorins pushed a commit that referenced this pull request Sep 10, 2017
This was a typo and accidentally returned the wrong value.

Fixes: #14819
Ref: #13467
PR-URL: #14821
Reviewed-By: Evan Lucas <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
MylesBorins pushed a commit that referenced this pull request Sep 11, 2017
This was a typo and accidentally returned the wrong value.

Fixes: #14819
Ref: #13467
PR-URL: #14821
Reviewed-By: Evan Lucas <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
MylesBorins pushed a commit that referenced this pull request Sep 12, 2017
This was a typo and accidentally returned the wrong value.

Fixes: #14819
Ref: #13467
PR-URL: #14821
Reviewed-By: Evan Lucas <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
buffer Issues and PRs related to the buffer subsystem. c++ Issues and PRs that require attention from people who are familiar with C++. 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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants