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#writeFloat/writeDouble methods don't return an error code yet while the others do #18115

Closed
rogierschouten opened this issue Jan 12, 2018 · 5 comments
Labels
buffer Issues and PRs related to the buffer subsystem.

Comments

@rogierschouten
Copy link

  • Version: 9.4.0
  • Platform: Windows 10 64-bit
  • Subsystem: buffer.js

Note how the first error has the ERR_INDEX_OUT_OF_RANGE code while the second one doesn't.

$ node
> var b = new Buffer([])
undefined
> b.writeInt8(3)
RangeError [ERR_INDEX_OUT_OF_RANGE]: Index out of range
    at checkInt (buffer.js:1277:11)
    at Buffer.writeInt8 (buffer.js:1442:5)
> b.writeDoubleBE(3)
RangeError: Index out of range
    at Buffer.writeDoubleBE (buffer.js:1533:5)
>

The affected methods are the write float/double LE/BE functions. All others behave correctly.

This complicates error handling unnecessarily and I don't think this is intended behavior. Bug?

@bnoordhuis bnoordhuis added the buffer Issues and PRs related to the buffer subsystem. label Jan 12, 2018
@bnoordhuis
Copy link
Member

The floating-point write methods are implemented in C++ and no C++ code currently throws ERR_ codes.

I'm planning to reimplement them in JS. I did that for their read counterparts in #17775 to good effect and it also fixes the error code thing in one swoop.

@seishun
Copy link
Contributor

seishun commented Feb 2, 2018

I'm planning to reimplement them in JS.

Are you still planning to do it any time soon? If not, I could give it a shot.

@bnoordhuis
Copy link
Member

@BridgeAR beat me to it, see #18395.

@apapirovski
Copy link
Member

Think this was fixed by #18395 but didn't get auto-closed. @BridgeAR @bnoordhuis please reopen if that's incorrect or some part was missed there.

@rogierschouten
Copy link
Author

@apapirovski I think you're right, this commit contains tests for the proper error behaviour: 63eb1fc

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.
Projects
None yet
Development

No branches or pull requests

4 participants