[libcxx-commits] [PATCH] D113027: [libcxx] Remove nonstandard _FilesystemClock::{to, from}_time_t

[Alexander Kornienko via Phabricator via libcxx-commits]

alexfh added a comment.

In D113027#3132727 <https://reviews.llvm.org/D113027#3132727>, @ldionne wrote:

> In D113027#3131369 <https://reviews.llvm.org/D113027#3131369>, @alexfh wrote:
>
>> In D113027#3125511 <https://reviews.llvm.org/D113027#3125511>, @mstorsjo wrote:
>>
>>> https://reviews.llvm.org/D113430 landed now, which should have implemented the standard methods instead.
>>
>> There is code out there, which already relies on these non-standard functions and it's going to be broken by this change without a reasonable alternative (migrating the code to C++20 is not a real alternative).
>
> Why is migrating to C++20 not a real alternative? Just curious.
>
> I would suggest we do this:
>
> 1. Add the methods back unless `_LIBCPP_ENABLE_REMOVED_FILE_CLOCK_TIME_T_METHODS` is defined.
> 2. Update the release note to mention that folks can use that macro to re-enable the methods temporarily, but that the option will be removed in two releases so they should move to C++20.
>
> Whoever is broken by this will see the breakage, then read the release notes, define `_LIBCPP_ENABLE_REMOVED_FILE_CLOCK_TIME_T_METHODS`, and make a note to move to C++20 before they are broken for good.

The plan above looks reasonable to me.

I looked a bit closer and found that performing conversion close to how the removed methods used to do this looks like an appropriate solution for our code: `std::filesystem::file_time_type(std::chrono::duration<std::filesystem::file_time_type::rep>(time_t_time))` and `std::time_t(std::chrono::duration_cast<std::chrono::duration<std::filesystem::file_time_type::rep>>(fs_time.time_since_epoch()).count())` seem to not rely on anything beyond C++17 standard. And while being more graceful when removing non-standard APIs still makes sense, we're likely not going to be blocked on this particular removal.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113027