[libcxx-commits] [PATCH] D91142: [10/N] [libcxx] Implement _FilesystemClock::now() and __last_write_time for windows

[Martin Storsjö via Phabricator via libcxx-commits]

mstorsjo added inline comments.


================
Comment at: libcxx/src/filesystem/operations.cpp:399
 #if defined(_LIBCPP_WIN32API)
 
 #define FILE_TIME_OFFSET_SECS (uint64_t(369 * 365 + 89) * (24 * 60 * 60))
----------------
amccarth wrote:
> Just a suggestion to give those magic values some context.
Good point, will apply that.


================
Comment at: libcxx/src/filesystem/operations.cpp:1227
+  if (!fs_time::convert_to_timespec(ts, new_time) ||
+      new_time == file_time_type::min())
+    return err.report(errc::value_too_large);
----------------
amccarth wrote:
> It's not clear to me why this checks `new_time == file_time_type::min()`.  It looks like convert_to_timespec returns false if the new time is not representable.  In what case would new_time be min?  And why would we then say the value is too large?
This is mostly a workaround to get this test working, when the filesystem time type isn't int128_t: https://github.com/llvm/llvm-project/blob/master/libcxx/test/std/input.output/filesystems/fs.op.funcs/fs.op.last_write_time/last_write_time.pass.cpp#L507-L540

But that test also relies on the test being able to guess what time values are reprepsentable and what aren't, and that doesn't really match what the implementation does anyway, so I guess I could just skip that testcase for now as well, and remove this odd hack here.

And saying the value is "too large" mostly means "out of range" I guess.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D91142/new/

https://reviews.llvm.org/D91142