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.

Sign up for GitHub

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

Jump to bottom

fs: add inoString to fs.Stats #16857

Closed
wants to merge 1 commit into from
Closed

fs: add inoString to fs.Stats #16857

wants to merge 1 commit into from

Conversation

XadillaX
Copy link
Contributor

@XadillaX XadillaX commented Nov 7, 2017 •
edited
Loading

Since ino is an unsigned 64-bit integer, it may lost accuracy in
JavaScript. This situation may cause some strange bugs in filesystem. So
a string type inoString is added to fs.Stats.

Fixes: #16679
Fixes: #12115
Refs: http://docs.libuv.org/en/v1.x/fs.html#c.uv_stat_t

Checklist
  • make -j4 test (UNIX)
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

fs

@XadillaX
fs: add inoString to fs.Stats
9ef7cc0 
Since `ino` is an unsigned 64-bit integer, it may lost accuracy in
JavaScript. This situation may cause some strange bugs in filesystem. So
a string type `inoString` is added to `fs.Stats`.

Fixes: nodejs#16679
Fixes: nodejs#12115
Refs: http://docs.libuv.org/en/v1.x/fs.html#c.uv_stat_t
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Nov 7, 2017
@XadillaX XadillaX added the fs Issues and PRs related to the fs subsystem / file system. label Nov 7, 2017
bnoordhuis
bnoordhuis requested changes Nov 7, 2017
View reviewed changes
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've commented on it elsewhere but IMO we should wait for BigInt support. It's at stage 3 and being implemented in V8.

@XadillaX
Copy link
Contributor Author

XadillaX commented Nov 7, 2017 •
edited
Loading

@bnoordhuis But how long will that support merge to Node.js? If it's a long time, people who submitted the issue how to resolve this problem so far?

vsemozhetbyt
vsemozhetbyt reviewed Nov 7, 2017
View reviewed changes
doc/api/fs.md
@@ -364,6 +365,11 @@ object alternate representations of the various times. The `Date` and number
values are not connected. Assigning a new number value, or mutating the `Date`
value, will not be reflected in the corresponding alternate representation.

*Note*: Since `ino` is an unsigned 64-bit integer, it may lost accuracy in
Copy link
Contributor

Choose a reason for hiding this comment

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

A nit: may lost -> may lose.

@bnoordhuis
Copy link
Member

bnoordhuis commented Nov 7, 2017 •
edited
Loading

But how long will that support merge to Node.js?

I expect it'll be ready for node 10.

@XadillaX
Copy link
Contributor Author

XadillaX commented Nov 7, 2017

@bnoordhuis IMHO I don't think they want to wait for so long a time since it's next LTS.

mscdex
mscdex suggested changes Nov 7, 2017
View reviewed changes
Copy link
Contributor

@mscdex mscdex left a comment

Choose a reason for hiding this comment

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

-1 We should just wait for BigInt

@XadillaX
Copy link
Contributor Author

XadillaX commented Nov 7, 2017

@mscdex @bnoordhuis how about to expose a raw uv_stats_t Buffer for developers selves use?

@mscdex
Copy link
Contributor

mscdex commented Nov 7, 2017

@XadillaX I'd rather see this done correctly from the get-go. We've already made it this far without many complaints, so I don't see the harm in waiting just a little bit longer for proper big number support in JavaScript.

@BridgeAR
Copy link
Member

Where are a couple of people against landing this and no one else has stood up for wanting this in.
Therefore I am going to close this. @XadillaX please reopen and ask the TSC if you feel like it should be discussed further.

@BridgeAR BridgeAR closed this Nov 24, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vsemozhetbyt vsemozhetbyt vsemozhetbyt left review comments

@bnoordhuis bnoordhuis bnoordhuis requested changes

@mscdex mscdex mscdex requested changes

Assignees
No one assigned
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. fs Issues and PRs related to the fs subsystem / file system. lib / src Issues and PRs related to general changes in the lib or src directory.
Projects
None yet
Milestone
No milestone
6 participants
@XadillaX @bnoordhuis @mscdex @BridgeAR @vsemozhetbyt @nodejs-github-bot