[libcxx] Remove nonstandard _FilesystemClock::{to,from}_time_t

[mstorsjo]

Summary

This follows up on D89679, where @ldionne suggested we should remove
these nonstandard functions altogether. In practice, one of them is
unused and the other one is only used in one single test. (D89679
marks those calls as libcxx specific to make the test runnable with
other C++ standard library implementations.)

[ldionne]

Can you please add a release note?

[mstorsjo]

Added release notes. Will push later.

[mstorsjo]

Retry CI

[mstorsjo]

Remove a leftover using Clock (which only was warned about in the GCC build)

[bgraur]

This revision removes the only available methods to convert std::chrono::file_time_type to/from anything else.

The standard says std::chrono::file_time_type::clock should have methods: to_sys() and from_sys() to convert to/from std::chrono::system_time.

Is it possible to revert this change until the standard methods are implemented?

[mstorsjo]

In D113027#3115431, @bgraur wrote:

This revision removes the only available methods to convert std::chrono::file_time_type to/from anything else.

The standard says std::chrono::file_time_type::clock should have methods: to_sys() and from_sys() to convert to/from std::chrono::system_time.

Is it possible to revert this change until the standard methods are implemented?

https://reviews.llvm.org/D113430 landed now, which should have implemented the standard methods instead.

[alexfh]

In D113027#3125511, @mstorsjo wrote:

In D113027#3115431, @bgraur wrote:

This revision removes the only available methods to convert std::chrono::file_time_type to/from anything else.

The standard says std::chrono::file_time_type::clock should have methods: to_sys() and from_sys() to convert to/from std::chrono::system_time.

Is it possible to revert this change until the standard methods are implemented?

https://reviews.llvm.org/D113430 landed now, which should have implemented the standard methods instead.

That doesn't help, since the standard methods are only available since C++20, while the ones removed here were available for earlier standards. 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). While I understand the desire to strictly follow the standard, it's also not nice to break existing code that relied on these features without a transition period. Can we have this reverted to give folks time to migrate code? To prevent backslide, these non-standard API fragments could be placed under an #ifdef and accompanied by a compiler warning.

[mstorsjo]

In D113027#3131369, @alexfh wrote:

In D113027#3125511, @mstorsjo wrote:

In D113027#3115431, @bgraur wrote:

This revision removes the only available methods to convert std::chrono::file_time_type to/from anything else.

The standard says std::chrono::file_time_type::clock should have methods: to_sys() and from_sys() to convert to/from std::chrono::system_time.

Is it possible to revert this change until the standard methods are implemented?

https://reviews.llvm.org/D113430 landed now, which should have implemented the standard methods instead.

That doesn't help, since the standard methods are only available since C++20, while the ones removed here were available for earlier standards. 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). While I understand the desire to strictly follow the standard, it's also not nice to break existing code that relied on these features without a transition period. Can we have this reverted to give folks time to migrate code? To prevent backslide, these non-standard API fragments could be placed under an #ifdef and accompanied by a compiler warning.

That sounds reasonable to me, but @ldionne has the final say on the matter.

How do you handle this with other C++ libraries, do they provide to_sys/from_sys even before C++20, or do they use some other nonstandard method, or were you only using this setup on libc++?

[rnk]

In D113027#3131371, @mstorsjo wrote:

How do you handle this with other C++ libraries, do they provide to_sys/from_sys even before C++20, or do they use some other nonstandard method, or were you only using this setup on libc++?

The code in question only uses libc++.

One thing that jumps out to me is that these methods were not uglified, not in the implementor's namespace, so users could reasonably consider them to be stable. To me, that seems like a good argument for reverting.

[ldionne]

In D113027#3131369, @alexfh wrote:

In 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.

[alexfh]

In D113027#3132727, @ldionne wrote:

In D113027#3131369, @alexfh wrote:

In 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.

libcxx/docs/ReleaseNotes.rst

- The call of the functions ``std::atomic_(add|sub)(std::atomic<T*>*, ...)``
   compiler are unaffected by this change.
 
 - The functions ``std::allocator<T>::allocate`` and
   ``std::experimental::pmr::polymorphic_allocator<T>::allocate`` now throw
   an exception of type ``std::bad_array_new_length`` when the requested size
   exceeds the maximum supported size, as required by the C++ standard.
   Previously the type ``std::length_error`` was used.
 
+- Removed the nonstandard methods ``std::chrono::file_clock::to_time_t`` and
+  ``std::chrono::file_clock::from_time_t``; neither libstdc++ nor MSVC STL
+  had such methods.
+
 ABI Changes
 -----------
 
 - The C++17 variable templates ``is_error_code_enum_v`` and
   ``is_error_condition_enum_v`` are now of type ``bool`` instead of ``size_t``.
 
 Build System Changes
 --------------------

libcxx/include/chrono

#endif
 
   typedef chrono::duration<rep, period> duration;
   typedef chrono::time_point<_FilesystemClock> time_point;
 
   _LIBCPP_EXPORTED_FROM_ABI
   static _LIBCPP_CONSTEXPR_AFTER_CXX11 const bool is_steady = false;
 
   _LIBCPP_AVAILABILITY_FILESYSTEM _LIBCPP_FUNC_VIS static time_point now() noexcept;
-
-  _LIBCPP_INLINE_VISIBILITY
-  static time_t to_time_t(const time_point& __t) noexcept {
-      typedef chrono::duration<rep> __secs;
-      return time_t(
-          chrono::duration_cast<__secs>(__t.time_since_epoch()).count());
-  }
-
-  _LIBCPP_INLINE_VISIBILITY
-  static time_point from_time_t(time_t __t) noexcept {
-      typedef chrono::duration<rep> __secs;
-      return time_point(__secs(__t));
-  }
 };
 _LIBCPP_END_NAMESPACE_FILESYSTEM
 #endif // !_LIBCPP_CXX03_LANG
 
 _LIBCPP_POP_MACROS
 
 #endif // _LIBCPP_CHRONO

libcxx/test/std/input.output/filesystems/fs.op.funcs/fs.op.last_write_time/last_write_time.pass.cpp

SleepFor(MilliSec(30));
         file_time_type ret2 = last_write_time(static_env.SymlinkToDir);
         TEST_CHECK(CompareTime(ret, ret2));
         TEST_CHECK(CompareTime(ret2, LastWriteTime(static_env.SymlinkToDir)));
     }
 }
 
 TEST_CASE(get_last_write_time_dynamic_env_test)
 {
-    using Clock = file_time_type::clock;
     using Sec = std::chrono::seconds;
     scoped_test_env env;
 
     const path file = env.create_file("file", 42);
     const path dir = env.create_dir("dir");
 
     const auto file_times = GetTimes(file);
     const TimeSpec file_write_time = file_times.write;
     const auto dir_times = GetTimes(dir);
     const TimeSpec dir_write_time = dir_times.write;
 
     file_time_type ftime = last_write_time(file);
-    TEST_CHECK(Clock::to_time_t(ftime) == file_write_time.tv_sec);
     TEST_CHECK(CompareTime(ftime, file_write_time));
 
     file_time_type dtime = last_write_time(dir);
-    TEST_CHECK(Clock::to_time_t(dtime) == dir_write_time.tv_sec);
     TEST_CHECK(CompareTime(dtime, dir_write_time));
 
     SleepFor(Sec(2));
 
     // update file and add a file to the directory. Make sure the times increase.
     std::FILE* of = std::fopen(file.string().c_str(), "a");
     std::fwrite("hello", 1, sizeof("hello"), of);
     std::fclose(of);