Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 34.2k
buffer: add constants with max buffer and string lengths#13467
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
Uh oh!
There was an error while loading. Please reload this page.
Conversation
vsemozhetbyt commented Jun 5, 2017
bnoordhuis left a comment
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.
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.)
src/node_buffer.cc Outdated
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.
Duplicate of kStringMaxLength. Suggestion: rename that one and update the tests that reference it.
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.
@bnoordhuis Huh, funny I didn’t see that. I used the other one anyway, so I’ve just removed the bits here again.
Fishrock123 commented Jun 5, 2017
seems like a good idea |
jasnell left a comment
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.
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 commented Jun 6, 2017
@jasnell This is meant to mirror the existing |
jasnell commented Jun 6, 2017
I would prefer it, yeah. The existing |
String::kMaxLengthAdd `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 commented Jun 7, 2017
@jasnell updated, please take a look :) @vsemozhetbyt@cjihrig you may want to re-review. |
cjihrig left a comment
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.
Still LGTM
vsemozhetbyt commented Jun 7, 2017 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
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 And maybe we can add some note to the 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 commented Jun 7, 2017
addaleax commented Jun 16, 2017
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 <[email protected]> Reviewed-By: Vse Mozhet Byt <[email protected]> Reviewed-By: Tobias Nießen <[email protected]>
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]>
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]>
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]>
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]>
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]>
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 <[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]>
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]>
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]>
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]>
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), orvcbuild test(Windows) passesAffected core subsystem(s)
buffer