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@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())); } } _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits