Needs a newline at the end of the file

Ack.

This should be a call, right?

Ohh yeah.

I think it would help with readability if you could add a comment explaining why these magic values are magic (I had to dig into the libuv sources, and I’m still not quite sure I got it right). ;)

Tryied to hint with the name, but will add a comment.

I think it would be a good idea if you could leave a comment here, explaining why this is needed.

And just so I get it right; this is needed because tv_sec overflows? I think the real problem in that case is that we don’t check for overflows in libuv during fs__utime_handle, which we probably should? Am I getting it totally wrong?

These changes seem unrelated, and I don’t think they are actually changing anything?

They remove warnings, but I'll kick them out since I have a more "warning reducing" PR cooking

That’s not really a comment explaining what’s happening…

It’s not like (signed long) 2159345162531 == -2135622133469 – it’s more complicated than that, it definitely relates to how the Windows API deals with timestamps.

It's almost that simple https://github.com/nodejs/node/blob/master/deps/uv/src/win/fs.c#L91
NTFS stores times in nanoseconds in a uint64_t but when uv converts it down to long we get 2's complement.

Hm. Then maybe we should cast back to unsigned only on Windows?

But really, I’m not sure whether this is the right approach, or if we are better off throwing in utimes if the time is not representable as a long…

I added to the comment why it's not a problem in POSIX, because there timestamps are stored in two fields struct {long t_sec, long t_usec}, and neither can overflow.
The problem is just how we convert from the NTFS uint64_t that hold nanoseconds, back to seconds.
IMHO This change is safe, since even the numbers from NTFS if properly converted to seconds fit in a long and in a double. (notice that the actual call to utimesSync divides these numbers by 1000). Also no FS will give us negative times, and the paper mentioned in the bug (interpretation-of-ntfs-timestamps), says we just need to interpret it right.

You might be right in that the calculations in https://github.com/nodejs/node/blob/master/deps/uv/src/win/fs.c#L91 should be done with uint

Y2k38 bug? ;) I think doing the math in uint64_t (or double because it’s getting converted anyway) as you had it before was fine, is there any reason to switch?

This whole fix turns Y2K38 to something like Y3K16
But there's a bug when doing static_cast<uint64_t> since the MSB is 1, it blows up to a huge number.
Also long (a.k.a. int32_t) to uint32_t is faster / stabler anyway.

*static_cast<uint64_t>

This whole fix turns Y2K38 to something like Y3K16

Ah, right. (Still not great, but yes, it’s better.)

But there's a bug when doing static_cast<uint64_t> since the MSB is 1, it blows up to a huge number.

You have the overflow problem anyway, though? Also, this doesn’t happen if you cast to double directly.

since casting to double is a calculation is comes out a negative double.
Maybe a reinterpret_cast<uint64_t> or static_cast<uint64_t>(static_cast<uint32_t>()) first inner one to convince the compiler it's a positive number...

@refack The thing is, I’m not sure you can count on long being 32-bit on all those odd systems that Node supports. unsigned long is the right thing anyway if you are reading from a long.

That was my initial thought but the linter was angry at me:
for
(static_cast<unsigned long>(s->st_##name.tv_sec) * 1e3)
it spet out
src/node_file.cc:483: Use int16/int64/etc, rather than the C type long [runtime/int] [4]
got the same for:
(unsigned long)(s->st_##name.tv_sec) * 1e3)

*angry

That was my initial thought

Good that we agree, though. :)

but the linter was angry at me:

// NOLINT(runtime/int) should help, although I’m not sure where exactly you need to place that

Done

s/uv/libuv/

Ack.