[libcxx-commits] [PATCH] D60491: Fix fpos requirements & cleanup

[Eric Fiselier via Phabricator via libcxx-commits]

EricWF requested changes to this revision.
EricWF added a comment.
This revision now requires changes to proceed.

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.



================
Comment at: include/string:557
+        {__off_ -= __off; return *this;}
+    _LIBCPP_INLINE_VISIBILITY fpos operator- (fpos<_StateT>& __rhs) const
+        {return fpos(__off_ - __rhs.__off_);}
----------------
`fpos<_StateT>` is the same as `fpos` in this context, no?


================
Comment at: include/string:559
+        {return fpos(__off_ - __rhs.__off_);}
+    _LIBCPP_INLINE_VISIBILITY fpos& operator= (fpos<_StateT> __rhs)
+        {_VSTD::swap(__rhs.__off_, __off_); return *this;}
----------------
This seems wrong. It's just another copy constructor overload, but that takes by value.




================
Comment at: test/std/strings/fpos/fpos.pass.cpp:1
+//===----------------------------------------------------------------------===//
+//
----------------
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.


================
Comment at: test/std/strings/fpos/fpos.pass.cpp:33
+    ASSERT_SAME_TYPE(std::streampos, std::fpos<std::char_traits<char>::state_type>);
+
+    std::streampos p0;
----------------
No test for the trivial copy constructor requirements?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60491/new/

https://reviews.llvm.org/D60491