Message ID | YMOqVPLt9IDNJbSi@redhat.com |
---|---|
State | New |
Headers | show |
Series | [committed] libstdc++: Fix filesystem::path comparisons for C++23 | expand |
Hi Jonathan, On Fri, 11 Jun 2021 at 20:25, Jonathan Wakely via Gcc-patches <gcc-patches@gcc.gnu.org> wrote: > > In C++23 there is a basic_string_view(Range&&) constructor, which is > constrained to take a range (specifically, a contiguous_range). When the > filesystem::path comparison operators call lhs.compare(rhs) the overload > taking a string_view is considered, which means checking whether path > satisfies the range concept. That satisfaction result changes depending > whether path::iterator is complete, which is ill-formed; no diagnostic > required. To avoid the problem, this change ensures that the overload > resolution is performed in a context where path::iterator is complete > and the range concept is satisfied. (The result of overload resolution > is always that the compare(const path&) overload is the best match, but > we still have to consider the compare(basic_string_view<value_type>) one > to decide if it even participates in overload resolution). > > For std::filesystem::path we can't define the comparison operators later > in the file, because they are hidden friends, so a new helper is > introduced that gets defined when everything else is complete. > > For std::experimental::filesystem::path we can just move the definitions > of the comparison operators later in the file. > > Signed-off-by: Jonathan Wakely <jwakely@redhat.com> > > libstdc++-v3/ChangeLog: > > * include/bits/fs_path.h (operator==, operator<=>): Use new > _S_compare function. > (path::_S_compare): New function to call path::compare in a > context where path::iterator is complete. > * include/experimental/bits/fs_path.h (operator<, operator==): > Define after path::iterator is complete. > * testsuite/27_io/filesystem/path/native/conv_c++23.cc: New > test. > * testsuite/experimental/filesystem/path/native/conv_c++23.cc: > New test. > > Tested powerpc64le-linux. Committed to trunk. > > This also needs to be backported to gcc-11 where the string_view > constructor is present for C++23 mode. > The new test experimental/filesystem/path/native/conv_c++23.cc fails on arm-eabi and aarch64-elf: /libstdc++-v3/testsuite/experimental/filesystem/path/native/conv_c++23.cc:4: fatal error: experimental/filesystem: No such file or directory (both trunk and gcc-11) I guess that's easy to fix? :-) Thanks, Christophe
On Mon, 14 Jun 2021 at 13:58, Christophe Lyon <christophe.lyon@linaro.org> wrote: > > Hi Jonathan, > > On Fri, 11 Jun 2021 at 20:25, Jonathan Wakely via Gcc-patches > <gcc-patches@gcc.gnu.org> wrote: > > > > In C++23 there is a basic_string_view(Range&&) constructor, which is > > constrained to take a range (specifically, a contiguous_range). When the > > filesystem::path comparison operators call lhs.compare(rhs) the overload > > taking a string_view is considered, which means checking whether path > > satisfies the range concept. That satisfaction result changes depending > > whether path::iterator is complete, which is ill-formed; no diagnostic > > required. To avoid the problem, this change ensures that the overload > > resolution is performed in a context where path::iterator is complete > > and the range concept is satisfied. (The result of overload resolution > > is always that the compare(const path&) overload is the best match, but > > we still have to consider the compare(basic_string_view<value_type>) one > > to decide if it even participates in overload resolution). > > > > For std::filesystem::path we can't define the comparison operators later > > in the file, because they are hidden friends, so a new helper is > > introduced that gets defined when everything else is complete. > > > > For std::experimental::filesystem::path we can just move the definitions > > of the comparison operators later in the file. > > > > Signed-off-by: Jonathan Wakely <jwakely@redhat.com> > > > > libstdc++-v3/ChangeLog: > > > > * include/bits/fs_path.h (operator==, operator<=>): Use new > > _S_compare function. > > (path::_S_compare): New function to call path::compare in a > > context where path::iterator is complete. > > * include/experimental/bits/fs_path.h (operator<, operator==): > > Define after path::iterator is complete. > > * testsuite/27_io/filesystem/path/native/conv_c++23.cc: New > > test. > > * testsuite/experimental/filesystem/path/native/conv_c++23.cc: > > New test. > > > > Tested powerpc64le-linux. Committed to trunk. > > > > This also needs to be backported to gcc-11 where the string_view > > constructor is present for C++23 mode. > > > > The new test experimental/filesystem/path/native/conv_c++23.cc > fails on arm-eabi and aarch64-elf: > /libstdc++-v3/testsuite/experimental/filesystem/path/native/conv_c++23.cc:4: > fatal error: experimental/filesystem: No such file or directory > (both trunk and gcc-11) > > I guess that's easy to fix? :-) Yes, fixed on trunk now by the attached patch. Tests running for the gcc-11 backport. commit a40d2293a7a417fc7711f8f4e52dbd4c48c2a43f Author: Jonathan Wakely <jwakely@redhat.com> Date: Mon Jun 14 14:22:07 2021 libstdc++: Only run Filesystem TS test if supported libstdc++-v3/ChangeLog: * testsuite/experimental/filesystem/path/native/conv_c++23.cc: Add dg-require-filesystem-ts directive. diff --git a/libstdc++-v3/testsuite/experimental/filesystem/path/native/conv_c++23.cc b/libstdc++-v3/testsuite/experimental/filesystem/path/native/conv_c++23.cc index f3c3f884965..81f57549ac9 100644 --- a/libstdc++-v3/testsuite/experimental/filesystem/path/native/conv_c++23.cc +++ b/libstdc++-v3/testsuite/experimental/filesystem/path/native/conv_c++23.cc @@ -1,5 +1,6 @@ // { dg-options "-std=gnu++23" } // { dg-do compile { target c++23 } } +// { dg-require-filesystem-ts "" } #include <experimental/filesystem>
diff --git a/libstdc++-v3/include/bits/fs_path.h b/libstdc++-v3/include/bits/fs_path.h index ad2518f414c..5e285204527 100644 --- a/libstdc++-v3/include/bits/fs_path.h +++ b/libstdc++-v3/include/bits/fs_path.h @@ -513,13 +513,13 @@ namespace __detail /// Compare paths friend bool operator==(const path& __lhs, const path& __rhs) noexcept - { return __lhs.compare(__rhs) == 0; } + { return path::_S_compare(__lhs, __rhs) == 0; } #if __cpp_lib_three_way_comparison /// Compare paths friend strong_ordering operator<=>(const path& __lhs, const path& __rhs) noexcept - { return __lhs.compare(__rhs) <=> 0; } + { return path::_S_compare(__lhs, __rhs) <=> 0; } #else /// Compare paths friend bool operator!=(const path& __lhs, const path& __rhs) noexcept @@ -627,6 +627,11 @@ namespace __detail static basic_string<_CharT, _Traits, _Allocator> _S_str_convert(basic_string_view<value_type>, const _Allocator&); + // Returns lhs.compare(rhs), but defined after path::iterator is complete. + __attribute__((__always_inline__)) + static int + _S_compare(const path& __lhs, const path& __rhs) noexcept; + void _M_split_cmpts(); _Type _M_type() const noexcept { return _M_cmpts.type(); } @@ -1330,6 +1335,14 @@ namespace __detail return _M_at_end == __rhs._M_at_end; } + // Define this now that path and path::iterator are complete. + // It needs to consider the string_view(Range&&) constructor during + // overload resolution, which depends on whether range<path> is satisfied, + // which depends on whether path::iterator is complete. + inline int + path::_S_compare(const path& __lhs, const path& __rhs) noexcept + { return __lhs.compare(__rhs); } + /// @} group filesystem _GLIBCXX_END_NAMESPACE_CXX11 } // namespace filesystem diff --git a/libstdc++-v3/include/experimental/bits/fs_path.h b/libstdc++-v3/include/experimental/bits/fs_path.h index 1ecf2f3a7bd..1fb84ec1e78 100644 --- a/libstdc++-v3/include/experimental/bits/fs_path.h +++ b/libstdc++-v3/include/experimental/bits/fs_path.h @@ -551,8 +551,7 @@ namespace __detail size_t hash_value(const path& __p) noexcept; /// Compare paths - inline bool operator<(const path& __lhs, const path& __rhs) noexcept - { return __lhs.compare(__rhs) < 0; } + inline bool operator<(const path& __lhs, const path& __rhs) noexcept; /// Compare paths inline bool operator<=(const path& __lhs, const path& __rhs) noexcept @@ -567,8 +566,7 @@ namespace __detail { return !(__lhs < __rhs); } /// Compare paths - inline bool operator==(const path& __lhs, const path& __rhs) noexcept - { return __lhs.compare(__rhs) == 0; } + inline bool operator==(const path& __lhs, const path& __rhs) noexcept; /// Compare paths inline bool operator!=(const path& __lhs, const path& __rhs) noexcept @@ -1275,6 +1273,16 @@ namespace __detail return _M_at_end == __rhs._M_at_end; } + // Define these now that path and path::iterator are complete. + // They needs to consider the string_view(Range&&) constructor during + // overload resolution, which depends on whether range<path> is satisfied, + // which depends on whether path::iterator is complete. + inline bool operator<(const path& __lhs, const path& __rhs) noexcept + { return __lhs.compare(__rhs) < 0; } + + inline bool operator==(const path& __lhs, const path& __rhs) noexcept + { return __lhs.compare(__rhs) == 0; } + /// @} group filesystem-ts _GLIBCXX_END_NAMESPACE_CXX11 } // namespace v1 diff --git a/libstdc++-v3/testsuite/27_io/filesystem/path/native/conv_c++23.cc b/libstdc++-v3/testsuite/27_io/filesystem/path/native/conv_c++23.cc new file mode 100644 index 00000000000..b4efed2bd47 --- /dev/null +++ b/libstdc++-v3/testsuite/27_io/filesystem/path/native/conv_c++23.cc @@ -0,0 +1,12 @@ +// { dg-options "-std=gnu++23" } +// { dg-do compile { target c++23 } } + +#include <filesystem> + +void +test01() +{ + using std::filesystem::path; + path p; + path::string_type s(p); +} diff --git a/libstdc++-v3/testsuite/experimental/filesystem/path/native/conv_c++23.cc b/libstdc++-v3/testsuite/experimental/filesystem/path/native/conv_c++23.cc new file mode 100644 index 00000000000..f3c3f884965 --- /dev/null +++ b/libstdc++-v3/testsuite/experimental/filesystem/path/native/conv_c++23.cc @@ -0,0 +1,12 @@ +// { dg-options "-std=gnu++23" } +// { dg-do compile { target c++23 } } + +#include <experimental/filesystem> + +void +test01() +{ + using std::experimental::filesystem::path; + path p; + path::string_type s(p); +}