Fix fpos requirements & cleanup

[zoecarver]

Summary

This patch implements P0759R1.

[zoecarver]

Update www

[EricWF]

I don't really understand how a lot of the changes in this patch implement the changes in the paper. Can you explain the rationale for each change?

The tests for each individual change need to be put in the exiting test files for the operations they modify.

I think this patch needs some work.

include/string

557

fpos<_StateT> is the same as fpos in this context, no?

559

This seems wrong. It's just another copy constructor overload, but that takes by value.

test/std/strings/fpos/fpos.pass.cpp

2

These tests should be integrated into the existing tests files under std/input.output/iostreams.base/fpos/fpos.operations.

The test name and directory correspond directly to the stable name of the section of the standard it is testing.

34

No test for the trivial copy constructor requirements?

[zoecarver]

I got confused about what type o was in the paper. This patch should conform to the paper properly. I will update the tests in the correct places but wanted to get this part out for review first.

[zoecarver]

include/string

563

The overloads conflict with the plus and minus overloads for streamoff. I think it would be fine to use those overloads (because they will convert both sides to streamoff), but I am also happy to define the behavior explicitly.

[zoecarver]

• update & add tests
• update synopsis
• clean up members / operators

[zoecarver]

include/string

542

I re-ordered and updated these operators and member functions. I don't think it changes any of the functionality so I am fine without the updates (say the word, and I will remove the changes).

[zoecarver]

• remove unneeded assignment operator
• move tests to correct location

(sorry for the repetitive updates)

[mclow.lists]

include/string

542

I think that's a good idea. This paper is supposed to be just wording cleanup; no changes to functionality, so lets make this patch just a test cleanup.

The re-ordering, etc of the member functions and operators can be done afterwards as a NFC patch.

[zoecarver]

include/string

542

Sounds good. I will remove this part of the patch and re-upload the diff.

[zoecarver]

• remove changes to fpos

[mclow.lists]

test/std/input.output/iostreams.base/fpos/fpos.operations/fpos.pass.cpp

20 ↗

(On Diff #200754)

Why is this unsupported? fpos was a thing in C++98/03.

34 ↗

(On Diff #200754)

error: static_assert with no message is a C++17 extension

[zoecarver]

test/std/input.output/iostreams.base/fpos/fpos.operations/fpos.pass.cpp

20 ↗

(On Diff #200754)

But the updates made here were not a thing until C++20. Should I still have the test for earlier versions? Or should I change this to only work after C++17?

[ldionne]

Some of the tests you added are already present in other files, for example libcxx/test/std/input.output/iostreams.base/fpos/fpos.operations/subtraction.pass.cpp. Any reason why you're duplicating those checks in test/std/input.output/iostreams.base/fpos/fpos.operations/fpos.pass.cpp?

test/std/input.output/iostreams.base/fpos/fpos.operations/fpos.pass.cpp

20 ↗

(On Diff #200754)

Since this is a DR, I think it's OK to assume the fix is there in prior standards. So you should remove the UNSUPPORTED annotation.

28 ↗

(On Diff #200754)

std::declval<T const&>() instead?

34 ↗

(On Diff #200754)

@zoecarver Action item: add a message!

54 ↗

(On Diff #200754)

I don't think you need any other types.

[zoecarver]

Yes, some of the tests are duplicated (subtraction, addition, comparison, etc.) but, I think it is nice to have it match the order of the table in the standard. Say the word and I'll remove the duplication, though.

[zoecarver]

• fix review comments

[ldionne]

libcxx/www/cxx2a_status.html

96 ↗

(On Diff #226669)

Can you mark it as being implemented in LLVM 11.0?

[zoecarver]

These tests should be integrated into the existing tests files under std/input.output/iostreams.base/fpos/fpos.operations.

@EricWF Do you still want me to split this up? I'm going to commit this patch as-is (with the change @ldionne asked for) but, if you still want me to split this test up, post a comment here and I'll revert, make the change, and put the patch here for re-review.

[zoecarver]

Committed as df73e36dc6ffd2ede500189b075f7ac52794b3ad. I can't close the revision for some reason (probably because of the blocking reviews).

include/string

 {
 
 template <class stateT>
 class fpos
 {
 private:
     stateT st;
 public:
-    fpos(streamoff = streamoff());
+    fpos();
+    fpos(streamoff);
 
     operator streamoff() const;
 
     stateT state() const;
     void state(stateT);
 
     fpos& operator+=(streamoff);
-    fpos  operator+ (streamoff) const;
+    fpos  operator+ (fpos&) const;
     fpos& operator-=(streamoff);
-    fpos  operator- (streamoff) const;
+    fpos  operator- (fpos&) const;
+    fpos& operator= (fpos);
 };
 
-template <class stateT> streamoff operator-(const fpos<stateT>& x, const fpos<stateT>& y);
-
 template <class stateT> bool operator==(const fpos<stateT>& x, const fpos<stateT>& y);
 template <class stateT> bool operator!=(const fpos<stateT>& x, const fpos<stateT>& y);
 
 template <class charT>
 struct char_traits
 {
     typedef charT     char_type;
     typedef ...       int_type;

 // fpos
 
 template <class _StateT>
 class _LIBCPP_TEMPLATE_VIS fpos
 {
 private:
     _StateT __st_;
     streamoff __off_;
 public:
-    _LIBCPP_INLINE_VISIBILITY fpos(streamoff __off = streamoff()) : __st_(), __off_(__off) {}
+    _LIBCPP_INLINE_VISIBILITY fpos()                : __st_(), __off_(0    ) {}
+    _LIBCPP_INLINE_VISIBILITY fpos(streamoff __off) : __st_(), __off_(__off) {}
 
     _LIBCPP_INLINE_VISIBILITY operator streamoff() const {return __off_;}
 
     _LIBCPP_INLINE_VISIBILITY _StateT state() const {return __st_;}
     _LIBCPP_INLINE_VISIBILITY void state(_StateT __st) {__st_ = __st;}
 
-    _LIBCPP_INLINE_VISIBILITY fpos& operator+=(streamoff __off) {__off_ += __off; return *this;}
-    _LIBCPP_INLINE_VISIBILITY fpos  operator+ (streamoff __off) const {fpos __t(*this); __t += __off; return __t;}
-    _LIBCPP_INLINE_VISIBILITY fpos& operator-=(streamoff __off) {__off_ -= __off; return *this;}
-    _LIBCPP_INLINE_VISIBILITY fpos  operator- (streamoff __off) const {fpos __t(*this); __t -= __off; return __t;}
+    _LIBCPP_INLINE_VISIBILITY fpos& operator+=(streamoff __off)
+        {__off_ += __off; return *this;}
+    _LIBCPP_INLINE_VISIBILITY fpos operator+ (fpos<_StateT>& __rhs) const
+        {return fpos(__rhs.__off_ + __off_);}
+    _LIBCPP_INLINE_VISIBILITY fpos& operator-=(streamoff __off)
+        {__off_ -= __off; return *this;}
+    _LIBCPP_INLINE_VISIBILITY fpos operator- (fpos<_StateT>& __rhs) const
+        {return fpos(__off_ - __rhs.__off_);}
+    _LIBCPP_INLINE_VISIBILITY fpos& operator= (fpos<_StateT> __rhs)
+        {_VSTD::swap(__rhs.__off_, __off_); return *this;}
 };
 
 template <class _StateT>
 inline _LIBCPP_INLINE_VISIBILITY
-streamoff operator-(const fpos<_StateT>& __x, const fpos<_StateT>& __y)
-    {return streamoff(__x) - streamoff(__y);}
-
-template <class _StateT>
-inline _LIBCPP_INLINE_VISIBILITY
-bool operator==(const fpos<_StateT>& __x, const fpos<_StateT>& __y)
-    {return streamoff(__x) == streamoff(__y);}
-
-template <class _StateT>
-inline _LIBCPP_INLINE_VISIBILITY
 bool operator!=(const fpos<_StateT>& __x, const fpos<_StateT>& __y)
     {return streamoff(__x) != streamoff(__y);}
 
 // basic_string
 
 template<class _CharT, class _Traits, class _Allocator>
 basic_string<_CharT, _Traits, _Allocator>
 operator+(const basic_string<_CharT, _Traits, _Allocator>& __x,

 template <class _Iter>
 struct __libcpp_string_gets_noexcept_iterator_impl : public true_type {};
 #elif defined(_LIBCPP_HAS_NO_NOEXCEPT)
 template <class _Iter>
 struct __libcpp_string_gets_noexcept_iterator_impl : public false_type {};
 #else
 template <class _Iter, bool = __is_forward_iterator<_Iter>::value>
 struct __libcpp_string_gets_noexcept_iterator_impl : public _LIBCPP_BOOL_CONSTANT((
-    noexcept(++(declval<_Iter&>())) && 
-    is_nothrow_assignable<_Iter&, _Iter>::value && 
-    noexcept(declval<_Iter>() == declval<_Iter>()) && 
+    noexcept(++(declval<_Iter&>())) &&
+    is_nothrow_assignable<_Iter&, _Iter>::value &&
+    noexcept(declval<_Iter>() == declval<_Iter>()) &&
     noexcept(*declval<_Iter>())
 )) {};
 
-template <class _Iter> 
+template <class _Iter>
 struct __libcpp_string_gets_noexcept_iterator_impl<_Iter, false> : public false_type {};
 #endif
 
 
 template <class _Iter>
 struct __libcpp_string_gets_noexcept_iterator
     : public _LIBCPP_BOOL_CONSTANT(__libcpp_is_trivial_iterator<_Iter>::value || __libcpp_string_gets_noexcept_iterator_impl<_Iter>::value) {};
 
#if _LIBCPP_STD_VER > 17
     _LIBCPP_CONSTEXPR_AFTER_CXX11 _LIBCPP_INLINE_VISIBILITY
     bool ends_with(const value_type* __s) const _NOEXCEPT
     { return ends_with(__self_view(__s)); }
 #endif
 
     _LIBCPP_INLINE_VISIBILITY bool __invariants() const;
 
     _LIBCPP_INLINE_VISIBILITY void __clear_and_shrink() _NOEXCEPT;
-    
     _LIBCPP_INLINE_VISIBILITY
     bool __is_long() const _NOEXCEPT
         {return bool(__r_.first().__s.__size_ & __short_mask);}
 
 #if _LIBCPP_DEBUG_LEVEL >= 2
 
     bool __dereferenceable(const const_iterator* __i) const;
     bool __decrementable(const const_iterator* __i) const;
template<class _CharT,
          class _Allocator = allocator<_CharT>,
          class = typename enable_if<__is_allocator<_Allocator>::value, void>::type,
          class _Sz = typename allocator_traits<_Allocator>::size_type
          >
 basic_string(basic_string_view<_CharT, _Traits>, _Sz, _Sz, const _Allocator& = _Allocator())
   -> basic_string<_CharT, _Traits, _Allocator>;
 #endif
 
-                  
 template <class _CharT, class _Traits, class _Allocator>
 inline
 void
 basic_string<_CharT, _Traits, _Allocator>::__invalidate_all_iterators()
 {
 #if _LIBCPP_DEBUG_LEVEL >= 2
     __get_db()->__invalidate_all(this);
 #endif  // _LIBCPP_DEBUG_LEVEL >= 2
basic_string<_CharT, _Traits, _Allocator>::__append_forward_unsafe(
     {
         typedef typename iterator_traits<_ForwardIterator>::reference _CharRef;
         _CharRef __tmp_ref = *__first;
         if (__ptr_in_range(_VSTD::addressof(__tmp_ref), data(), data() + size()))
         {
             const basic_string __temp (__first, __last, __alloc());
             append(__temp.data(), __temp.size());
         }
-        else 
+        else
         {
             if (__cap - __sz < __n)
                 __grow_by(__cap, __sz + __n - __cap, __sz, __sz, 0);
             pointer __p = __get_pointer() + __sz;
             for (; __first != __last; ++__p, ++__first)
                 traits_type::assign(*__p, *__first);
             traits_type::assign(*__p, value_type());
             __set_size(__sz + __n);
if (data()[size()] != value_type(0))
         return false;
     return true;
 }
 
 // __clear_and_shrink
 
 template<class _CharT, class _Traits, class _Allocator>
 inline
-void 
+void
 basic_string<_CharT, _Traits, _Allocator>::__clear_and_shrink() _NOEXCEPT
 {
     clear();
     if(__is_long())
     {
         __alloc_traits::deallocate(__alloc(), __get_long_pointer(), capacity() + 1);
         __set_long_cap(0);
         __set_short_size(0);
     }
-} 
+}
 
 // operator==
 
 template<class _CharT, class _Traits, class _Allocator>
 inline _LIBCPP_INLINE_VISIBILITY
 bool
 operator==(const basic_string<_CharT, _Traits, _Allocator>& __lhs,
            const basic_string<_CharT, _Traits, _Allocator>& __rhs) _NOEXCEPT
basic_string<_CharT, _Traits, _Allocator>::__subscriptable(const const_iterator* __i, ptrdiff_t __n) const
     return this->data() <= __p && __p < this->data() + this->size();
 }
 
 #endif  // _LIBCPP_DEBUG_LEVEL >= 2
 
 _LIBCPP_EXTERN_TEMPLATE(class _LIBCPP_EXTERN_TEMPLATE_TYPE_VIS basic_string<char>)
 _LIBCPP_EXTERN_TEMPLATE(class _LIBCPP_EXTERN_TEMPLATE_TYPE_VIS basic_string<wchar_t>)
 
-#if _LIBCPP_STD_VER > 11 
+#if _LIBCPP_STD_VER > 11
 // Literal suffixes for basic_string [basic.string.literals]
 inline namespace literals
 {
   inline namespace string_literals
   {
     inline _LIBCPP_INLINE_VISIBILITY
     basic_string<char> operator "" s( const char *__str, size_t __len )
     {

test/std/strings/fpos/fpos.pass.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
+//
+//===----------------------------------------------------------------------===//
+
+#include "test_macros.h"
+
+#include <string>
+#include <cassert>
+
+// string
+// template <class stateT>
+// class fpos
+// {
+//     fpos();
+//     fpos(streamoff);
+//
+//     operator streamoff() const;
+//
+//     fpos& operator+=(streamoff);
+//     fpos  operator+ (fpos&) const;
+//     fpos& operator-=(streamoff);
+//     fpos  operator- (fpos&) const;
+//     fpos& operator= (fpos);
+// };
+
+int main(int, char**)
+{
+    ASSERT_SAME_TYPE(std::streampos, std::fpos<std::char_traits<char>::state_type>);
+
+    std::streampos p0;
+    assert(std::streamoff(p0) == 0);
+
+    std::streampos p1(1);
+    assert(std::streamoff(p1) == 1);
+
+    std::streampos p2 = 2;
+    assert(std::streamoff(p2) == 2);
+
+    assert(p2 != p1);
+
+    assert(std::streamoff(p1 + p2) == 3);
+    assert(std::streamoff(p2 - p1) == 1);
+
+    p0 += 5;
+    assert(std::streamoff(p0) == 5);
+    p0 -= 2;
+    assert(std::streamoff(p0) == 3);
+
+    assert((p0 + 3) == 6);
+    assert((p0 - 3) == 0);
+
+    return 0;
+}