[libcxx] r300164 - Diagnose when reverse_iterator is used on path::iterator.

[Eric Fiselier via cfe-commits]

Author: ericwf
Date: Wed Apr 12 21:54:13 2017
New Revision: 300164

URL: http://llvm.org/viewvc/llvm-project?rev=300164&view=rev
Log:
Diagnose when reverse_iterator is used on path::iterator.

path::iterator isn't a strictly conforming iterator. Specifically
it stashes the current element inside the iterator. This leads to
UB when used with reverse_iterator since it requires the element
to outlive the lifetime of the iterator.

This patch adds a static_assert inside reverse_iterator to disallow
"stashing iterator types", and it tags path::iterator as such a type.

Additionally this patch removes all uses of reverse_iterator<path::iterator>
within the tests.

Added:
    libcxx/trunk/test/libcxx/experimental/filesystem/class.path/path.itr/reverse_iterator_produces_diagnostic.fail.cpp
Modified:
    libcxx/trunk/include/experimental/filesystem
    libcxx/trunk/include/iterator
    libcxx/trunk/test/std/experimental/filesystem/class.path/path.member/path.decompose/path.decompose.pass.cpp

Modified: libcxx/trunk/include/experimental/filesystem
URL: http://llvm.org/viewvc/llvm-project/libcxx/trunk/include/experimental/filesystem?rev=300164&r1=300163&r2=300164&view=diff
==============================================================================
--- libcxx/trunk/include/experimental/filesystem (original)
+++ libcxx/trunk/include/experimental/filesystem Wed Apr 12 21:54:13 2017
@@ -1092,20 +1092,12 @@ class _LIBCPP_TYPE_VIS path::iterator
 public:
     typedef bidirectional_iterator_tag iterator_category;
 
-    // FIXME: As of 3/April/2017 Clang warns on `value_type` shadowing the
-    // definition in path. Clang should be fixed and this should be removed.
-#if defined(__clang__)
-#pragma clang diagnostic push
-#pragma clang diagnostic ignored "-Wshadow"
-#endif
     typedef path                       value_type;
-#if defined(__clang__)
-#pragma clang diagnostic pop
-#endif
-
     typedef std::ptrdiff_t             difference_type;
     typedef const path*                pointer;
     typedef const path&                reference;
+
+    typedef void __stashing_iterator_tag; // See reverse_iterator and __is_stashing_iterator
 public:
     _LIBCPP_INLINE_VISIBILITY
     iterator() : __stashed_elem_(), __path_ptr_(nullptr),

Modified: libcxx/trunk/include/iterator
URL: http://llvm.org/viewvc/llvm-project/libcxx/trunk/include/iterator?rev=300164&r1=300163&r2=300164&view=diff
==============================================================================
--- libcxx/trunk/include/iterator (original)
+++ libcxx/trunk/include/iterator Wed Apr 12 21:54:13 2017
@@ -615,6 +615,14 @@ prev(_BidiretionalIter __x,
     return __x;
 }
 
+
+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 {};
+
 template <class _Iter>
 class _LIBCPP_TEMPLATE_VIS reverse_iterator
     : public iterator<typename iterator_traits<_Iter>::iterator_category,
@@ -625,6 +633,11 @@ class _LIBCPP_TEMPLATE_VIS reverse_itera
 {
 private:
     /*mutable*/ _Iter __t;  // no longer used as of LWG #2360, not removed due to ABI break
+
+    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:

Added: libcxx/trunk/test/libcxx/experimental/filesystem/class.path/path.itr/reverse_iterator_produces_diagnostic.fail.cpp
URL: http://llvm.org/viewvc/llvm-project/libcxx/trunk/test/libcxx/experimental/filesystem/class.path/path.itr/reverse_iterator_produces_diagnostic.fail.cpp?rev=300164&view=auto
==============================================================================
--- libcxx/trunk/test/libcxx/experimental/filesystem/class.path/path.itr/reverse_iterator_produces_diagnostic.fail.cpp (added)
+++ libcxx/trunk/test/libcxx/experimental/filesystem/class.path/path.itr/reverse_iterator_produces_diagnostic.fail.cpp Wed Apr 12 21:54:13 2017
@@ -0,0 +1,31 @@
+//===----------------------------------------------------------------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is dual licensed under the MIT and the University of Illinois Open
+// Source Licenses. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+// UNSUPPORTED: c++98, c++03
+
+// <experimental/filesystem>
+
+// class path
+
+#include <experimental/filesystem>
+#include <iterator>
+
+
+namespace fs = std::experimental::filesystem;
+
+int main() {
+  using namespace fs;
+  using RIt = std::reverse_iterator<path::iterator>;
+
+  // expected-error at iterator:* {{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);
+  }
+}

Modified: libcxx/trunk/test/std/experimental/filesystem/class.path/path.member/path.decompose/path.decompose.pass.cpp
URL: http://llvm.org/viewvc/llvm-project/libcxx/trunk/test/std/experimental/filesystem/class.path/path.member/path.decompose/path.decompose.pass.cpp?rev=300164&r1=300163&r2=300164&view=diff
==============================================================================
--- libcxx/trunk/test/std/experimental/filesystem/class.path/path.member/path.decompose/path.decompose.pass.cpp (original)
+++ libcxx/trunk/test/std/experimental/filesystem/class.path/path.member/path.decompose/path.decompose.pass.cpp Wed Apr 12 21:54:13 2017
@@ -54,12 +54,6 @@
 #include "count_new.hpp"
 #include "filesystem_test_helper.hpp"
 
-template <class It>
-std::reverse_iterator<It> mkRev(It it) {
-  return std::reverse_iterator<It>(it);
-}
-
-
 namespace fs = std::experimental::filesystem;
 struct PathDecomposeTestcase
 {
@@ -147,7 +141,11 @@ void decompPathTest()
     assert(checkCollectionsEqual(p.begin(), p.end(),
                                  TC.elements.begin(), TC.elements.end()));
     // check backwards
-    assert(checkCollectionsEqual(mkRev(p.end()), mkRev(p.begin()),
+
+    std::vector<fs::path> Parts;
+    for (auto it = p.end(); it != p.begin(); )
+      Parts.push_back(*--it);
+    assert(checkCollectionsEqual(Parts.begin(), Parts.end(),
                                  TC.elements.rbegin(), TC.elements.rend()));
   }
 }