This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Use __int128_t to represent file_time_type.
ClosedPublic

Authored by EricWF on Jul 24 2018, 8:00 PM.

Details

Summary

The `file_time_type` time point is used to represent the write times for files.
Its job is to act as part of a C++ wrapper for less ideal system interfaces. The
underlying filesystem uses the `timespec` struct for the same purpose.

However, the initial implementation of `file_time_type` could not represent
either the range or resolution of `timespec`, making it unsuitable. Fixing
this requires an implementation which uses more than 64 bits to store the
time point.

I primarily considered two solutions: Using `__int128_t` and using a
arithmetic emulation of `timespec`. Each has its pros and cons, and both
come with more than one complication.

However, after a lot of consideration, I decided on using __int128_t. This patch implements that change.

Please see the FileTimeType Design Document for more information.

Diff Detail

Repository
rCXX libc++

Event Timeline

EricWF created this revision.Jul 24 2018, 8:00 PM

This is the final change I need to land before adding <experimental/filesystem>, so I don't plan to let this review linger long.

If you have any strong objections, please speak now or forever hold your peace (or at least until the next ABI break, whichever comes first).

I haven't reviewed this closely, but you might want to look at http://wg21.link/P0355, where we added a file_clock and file_time types.

I haven't reviewed this closely, but you might want to look at http://wg21.link/P0355, where we added a file_clock and file_time types.

Thanks for the information. It doesn't look to have too much bearing on this from a design standpoint. Except to note that it seems to solve
the streaming problem by adding explicit streaming overload for time points.

I haven't reviewed this closely, but you might want to look at http://wg21.link/P0355, where we added a file_clock and file_time types.

Thanks for the information. It doesn't look to have too much bearing on this from a design standpoint. Except to note that it seems to solve
the streaming problem by adding explicit streaming overload for time points.

It doesn't change anything from a design standpoint, but I don't think we can ship something deemed stable without taking P0355 into account. That's because it adds file_clock and changes file_time_type to use it, which is an ABI break. So if we take filesystem out of experimental and ship it before we've actually implemented the parts of P0355 that change the ABI, we'll have to take an ABI break in the future. That would suck because this ABI break would be necessary to implement C++20 properly in a fairly major way.

That's my understanding. Appart from that, I've read the design docs and while I'm not very familiar with the problems involved, I follow the reasoning and I think using __int128_t makes at least as much sense as the other potential solutions.

LGTM

I haven't reviewed this closely, but you might want to look at http://wg21.link/P0355, where we added a file_clock and file_time types.

Thanks for the information. It doesn't look to have too much bearing on this from a design standpoint. Except to note that it seems to solve
the streaming problem by adding explicit streaming overload for time points.

It doesn't change anything from a design standpoint, but I don't think we can ship something deemed stable without taking P0355 into account. That's because it adds file_clock and changes file_time_type to use it, which is an ABI break. So if we take filesystem out of experimental and ship it before we've actually implemented the parts of P0355 that change the ABI, we'll have to take an ABI break in the future. That would suck because this ABI break would be necessary to implement C++20 properly in a fairly major way.

Another problem (that Eric and I discussed last night) is that filesystem is part of C++17, and file_clock is C++20. So we need a solution for C++17 as well.

I haven't reviewed this closely, but you might want to look at http://wg21.link/P0355, where we added a file_clock and file_time types.

Thanks for the information. It doesn't look to have too much bearing on this from a design standpoint. Except to note that it seems to solve
the streaming problem by adding explicit streaming overload for time points.

It doesn't change anything from a design standpoint, but I don't think we can ship something deemed stable without taking P0355 into account. That's because it adds file_clock and changes file_time_type to use it, which is an ABI break. So if we take filesystem out of experimental and ship it before we've actually implemented the parts of P0355 that change the ABI, we'll have to take an ABI break in the future. That would suck because this ABI break would be necessary to implement C++20 properly in a fairly major way.

Another problem (that Eric and I discussed last night) is that filesystem is part of C++17, and file_clock is C++20. So we need a solution for C++17 as well.

I believe one approach here would be to define __file_clock in C++17 (the way it is specified in C++20), and then have using file_clock = __file_clock in C++20. Would this be a valid strategy?

Another problem (that Eric and I discussed last night) is that filesystem is part of C++17, and file_clock is C++20. So we need a solution for C++17 as well.

It seems like we need to fix C++20 to allow that to be a typedef to a type in std::filesystem or that will be ABI breaking for MSVC++. IMO we should fix the spec to allow that rather than make libc++ jump through insane hoops.

Another problem (that Eric and I discussed last night) is that filesystem is part of C++17, and file_clock is C++20. So we need a solution for C++17 as well.

It seems like we need to fix C++20 to allow that to be a typedef to a type in std::filesystem or that will be ABI breaking for MSVC++. IMO we should fix the spec to allow that rather than make libc++ jump through insane hoops.

We could also just provide file_clock "early" in C++17. Strictly speaking that may make our implementation non-conforming, but IDK how big of a deal this would be?

Another problem (that Eric and I discussed last night) is that filesystem is part of C++17, and file_clock is C++20. So we need a solution for C++17 as well.

It seems like we need to fix C++20 to allow that to be a typedef to a type in std::filesystem or that will be ABI breaking for MSVC++. IMO we should fix the spec to allow that rather than make libc++ jump through insane hoops.

We could also just provide file_clock "early" in C++17. Strictly speaking that may make our implementation non-conforming, but IDK how big of a deal this would be?

Since I've been working on implementing P0355 for C++20, I'll work on file_clock tonight.

Another problem (that Eric and I discussed last night) is that filesystem is part of C++17, and file_clock is C++20. So we need a solution for C++17 as well.

It seems like we need to fix C++20 to allow that to be a typedef to a type in std::filesystem or that will be ABI breaking for MSVC++. IMO we should fix the spec to allow that rather than make libc++ jump through insane hoops.

We could also just provide file_clock "early" in C++17. Strictly speaking that may make our implementation non-conforming, but IDK how big of a deal this would be?

Sure, libc++ could do that. But because msvc++ and libstdc++ have already shipped this thing I think the spec will have to change.

No real user cares about the associated namespaces of a clock anyway.

Another problem (that Eric and I discussed last night) is that filesystem is part of C++17, and file_clock is C++20. So we need a solution for C++17 as well.

It seems like we need to fix C++20 to allow that to be a typedef to a type in std::filesystem or that will be ABI breaking for MSVC++. IMO we should fix the spec to allow that rather than make libc++ jump through insane hoops.

We could also just provide file_clock "early" in C++17. Strictly speaking that may make our implementation non-conforming, but IDK how big of a deal this would be?

Sure, libc++ could do that. But because msvc++ and libstdc++ have already shipped this thing I think the spec will have to change.

No real user cares about the associated namespaces of a clock anyway.

Agreed. I don't think this spec will land as-is.

test/std/experimental/filesystem/fs.op.funcs/fs.op.last_write_time/last_write_time.pass.cpp
583

TODO: remove this.

EricWF updated this revision to Diff 157349.Jul 25 2018, 1:36 PM

Update with misc cleanups.

EricWF accepted this revision.Jul 25 2018, 1:47 PM

I'm going to go ahead and commit this now. We can address concerns about the future C++20 spec in follow up commits.

This revision is now accepted and ready to land.Jul 25 2018, 1:47 PM
This revision was automatically updated to reflect the committed changes.