[libc++] [API BREAK] Change `fs::path::iterator::iterator_category` to `input_iterator_tag`.

[• Quuxplusone]

Summary

This essentially reverts e02ed1c255d71 and puts in a new fix, which makes path::iterator a true C++20 bidirectional_iterator, but downgrades it to an input_iterator in C++17.

Fixes https://github.com/llvm/llvm-project/issues/37852

However, notice that this makes *it do a copy (of a hopefully short string), rather than just returning a reference; that might have perf implications.
And notice that a for-loop like for (auto& elt : path) will no longer compile (but this is just more evidence that auto&& Always Works ;)).
On the plus side, after this patch there is no downside to using std::reverse_iterator<fs::path::iterator>... unless I've missed something.

[ldionne]

I am fine with this, but it needs a release note under API changes. Also, it's technically possible for this to be an ABI break (if someone had path::iterator::reference in their ABI). However, if they do, it would cause a mangling issue so it would be caught at compile-time, which makes this acceptable to me ABI-wise.

Before I really give this a ship-it, let me run it on a large code base. We might discover some interesting fallout.

libcxx/test/std/input.output/filesystems/class.path/path.itr/iterator.pass.cpp

35

[• Quuxplusone]

@ldionne ping!

[ldionne]

Note to self: 87400598.

I'll ping this again in a couple of days once I have more information.

[ldionne]

Built a significant codebase and didn't see any issues. It doesn't mean that it won't cause *any* issues at all in the wild (I'm sure it will bite at least a few people), but at this point I don't see anything serious enough to prevent us from shipping this bugfix.

libcxx/docs/ReleaseNotes.rst

109

[• Quuxplusone]

libcxx/docs/ReleaseNotes.rst

109

Before this patch, it was rejected in C++17 and C++20 (and C++14, if you used std::__fs::filesystem::path::iterator to get at it).
After this patch, it is rejected in neither C++17 nor C++20.
(Ah, you thought it would be rejected in C++17 because path::iterator is an input iterator, not a bidirectional iterator, in C++17? Well, C++17 reverse_iterator doesn't care what your iterator category is. :) In C++17, it has an operator-- that DTRT, so it works.)

libcxx/docs/ReleaseNotes.rst

- Removed the nonstandard methods ``std::chrono::file_clock::to_time_t`` and
   and ``std::chrono::file_clock::to_sys``, which are specified in the Standard.
   If you are not using C++20, you should move to it.
 
 - The declarations of functions ``declare_reachable``, ``undeclare_reachable``, ``declare_no_pointers``,
   ``undeclare_no_pointers``, and ``get_pointer_safety`` have been removed not only from C++2b but
   from all modes. Their symbols are still provided by the dynamic library for the benefit of
   existing compiled code. All of these functions have always behaved as no-ops.
 
+- ``std::filesystem::path::iterator``, which (in our implementation) stashes
+  a ``path`` value inside itself similar to ``istream_iterator``, now sets its
+  ``reference`` type to ``path`` and its ``iterator_category`` to ``input_iterator_tag``,
+  so that it is a conforming input iterator in C++17 and a conforming
+  ``std::bidirectional_iterator`` in C++20. Before this release, it had set its
+  ``reference`` type to ``const path&`` and its ``iterator_category`` to
+  ``bidirectional_iterator_tag``, making it a non-conforming bidirectional iterator.
+  After this change, ``for`` loops of the form ``for (auto& c : path)`` must be rewritten
+  as either ``for (auto&& c : path)`` or ``for (const auto& c : path)``.
+  ``std::reverse_iterator<path::iterator>`` is no longer rejected.
+
+
 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``.
 
 - The C++03 emulation type for ``std::nullptr_t`` has been removed in favor of
   using ``decltype(nullptr)`` in all standard modes. This is an ABI break for

libcxx/include/__filesystem/path_iterator.h

enum _ParserState : unsigned char {
     _InRootName,
     _InRootDir,
     _InFilenames,
     _InTrailingSep,
     _AtEnd
   };
 
 public:
-  typedef bidirectional_iterator_tag iterator_category;
+  typedef input_iterator_tag iterator_category;
+  typedef bidirectional_iterator_tag iterator_concept;
 
   typedef path value_type;
   typedef ptrdiff_t difference_type;
   typedef const path* pointer;
-  typedef const path& reference;
-
-  typedef void
-      __stashing_iterator_tag; // See reverse_iterator and __is_stashing_iterator
+  typedef path reference;
 
 public:
   _LIBCPP_INLINE_VISIBILITY
   iterator()
       : __stashed_elem_(), __path_ptr_(nullptr), __entry_(),
         __state_(_Singular) {}
 
   iterator(const iterator&) = default;

libcxx/include/__iterator/reverse_iterator.h

 #include <type_traits>
 
 #if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
 #pragma GCC system_header
 #endif
 
 _LIBCPP_BEGIN_NAMESPACE_STD
 
-template <class _Tp, class = void>
-struct __is_stashing_iterator : false_type {};
-
-template <class _Tp>
-struct __is_stashing_iterator<_Tp, typename __void_t<typename _Tp::__stashing_iterator_tag>::type>
-  : true_type {};
-
 _LIBCPP_SUPPRESS_DEPRECATED_PUSH
 template <class _Iter>
 class _LIBCPP_TEMPLATE_VIS reverse_iterator
 #if _LIBCPP_STD_VER <= 14 || !defined(_LIBCPP_ABI_NO_ITERATOR_BASES)
     : public iterator<typename iterator_traits<_Iter>::iterator_category,
                       typename iterator_traits<_Iter>::value_type,
                       typename iterator_traits<_Iter>::difference_type,
                       typename iterator_traits<_Iter>::pointer,
                       typename iterator_traits<_Iter>::reference>
 #endif
 {
 _LIBCPP_SUPPRESS_DEPRECATED_POP
 private:
 #ifndef _LIBCPP_ABI_NO_ITERATOR_BASES
     _Iter __t; // no longer used as of LWG #2360, not removed due to ABI break
 #endif
 
-    static_assert(!__is_stashing_iterator<_Iter>::value,
-      "The specified iterator type cannot be used with reverse_iterator; "
-      "Using stashing iterators with reverse_iterator causes undefined behavior");
-
 protected:
     _Iter current;
 public:
     typedef _Iter                                            iterator_type;
     typedef typename iterator_traits<_Iter>::difference_type difference_type;
     typedef typename iterator_traits<_Iter>::reference       reference;
     typedef typename iterator_traits<_Iter>::pointer         pointer;
     typedef _If<__is_cpp17_random_access_iterator<_Iter>::value,

libcxx/test/libcxx/input.output/filesystems/class.path/path.itr/reverse_iterator_produces_diagnostic.verify.cpp

-//===----------------------------------------------------------------------===//
-//
-// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
-// See https://llvm.org/LICENSE.txt for license information.
-// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
-//
-//===----------------------------------------------------------------------===//
-
-// UNSUPPORTED: c++03
-
-// <filesystem>
-
-// class path
-
-#include "filesystem_include.h"
-#include <iterator>
-
-
-int main(int, char**) {
-  using namespace fs;
-  using RIt = std::reverse_iterator<path::iterator>;
-
-  // expected-error-re@*:* {{static_assert failed{{.*}} "The specified iterator type cannot be used with reverse_iterator; Using stashing iterators with reverse_iterator causes undefined behavior"}}
-  {
-    RIt r;
-    ((void)r);
-  }
-
-  return 0;
-}

libcxx/test/std/input.output/filesystems/class.path/path.itr/iterator.pass.cpp

 
 // template <class Source>
 //      path(const Source& source);
 // template <class InputIterator>
 //      path(InputIterator first, InputIterator last);
 
 
 #include "filesystem_include.h"
+#include <cassert>
 #include <iterator>
 #include <type_traits>
-#include <cassert>
 
 #include "test_macros.h"
 #include "filesystem_test_helper.h"
 
-
-
-template <class It>
-std::reverse_iterator<It> mkRev(It it) {
-  return std::reverse_iterator<It>(it);
-}
-
 void checkIteratorConcepts() {
   using namespace fs;
   using It = path::iterator;
   using Traits = std::iterator_traits<It>;
-  ASSERT_SAME_TYPE(Traits::iterator_category, std::bidirectional_iterator_tag);
+  ASSERT_SAME_TYPE(path::const_iterator, It);
+#if TEST_STD_VER > 17 && !defined(_LIBCPP_HAS_NO_RANGES)
+  static_assert(std::bidirectional_iterator<It>);
+#endif
   ASSERT_SAME_TYPE(Traits::value_type, path);
-  ASSERT_SAME_TYPE(Traits::pointer,   path const*);
-  ASSERT_SAME_TYPE(Traits::reference, path const&);
+  LIBCPP_STATIC_ASSERT(std::is_same<Traits::iterator_category, std::input_iterator_tag>::value, "");
+  LIBCPP_STATIC_ASSERT(std::is_same<Traits::pointer, path const*>::value, "");
+  LIBCPP_STATIC_ASSERT(std::is_same<Traits::reference, path>::value, "");
   {
     It it;
     ASSERT_SAME_TYPE(It&, decltype(++it));
     ASSERT_SAME_TYPE(It, decltype(it++));
     ASSERT_SAME_TYPE(It&, decltype(--it));
     ASSERT_SAME_TYPE(It, decltype(it--));
     ASSERT_SAME_TYPE(Traits::reference, decltype(*it));
     ASSERT_SAME_TYPE(Traits::pointer, decltype(it.operator->()));
using It = path::iterator;
     path p("//root_name//first_dir////second_dir");
 #ifdef _WIN32
     const path expect[] = {"//root_name", "/", "first_dir", "second_dir"};
 #else
     const path expect[] = {"/", "root_name", "first_dir", "second_dir"};
 #endif
     assert(checkCollectionsEqual(p.begin(), p.end(), std::begin(expect), std::end(expect)));
     assert(checkCollectionsEqualBackwards(p.begin(), p.end(), std::begin(expect), std::end(expect)));
-
   }
   {
     path p("////foo/bar/baz///");
     const path expect[] = {"/", "foo", "bar", "baz", ""};
     assert(checkCollectionsEqual(p.begin(), p.end(), std::begin(expect), std::end(expect)));
     assert(checkCollectionsEqualBackwards(p.begin(), p.end(), std::begin(expect), std::end(expect)));
-
   }
-
 }
 
 int main(int, char**) {
   using namespace fs;
   checkIteratorConcepts();
   checkBeginEndBasic(); // See path.decompose.pass.cpp for more tests.
 
   return 0;
 }

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

 #include "test_macros.h"
 #include "count_new.h"
 #include "rapid-cxx-test.h"
 #include "filesystem_test_helper.h"
 
 
 static int count_path_elems(const fs::path& p) {
   int count = 0;
-  for (auto& elem : p) {
+  for (auto&& elem : p) {
     if (elem != p.root_name() && elem != "/" && elem != "")
       ++count;
   }
   return count;
 }
 
 TEST_SUITE(filesystem_proximate_path_test_suite)