[libcxx-commits] [PATCH] D122257: [libc++] Fix undefined behavior in `std::filebuf`

[Fabian Wolff via Phabricator via libcxx-commits]

fwolff marked 5 inline comments as done.
fwolff added a comment.

Thanks for reviewing this @ldionne!



================
Comment at: libcxx/include/fstream:437
+            __re = __rhs.__extbufend_ - __rhs.__extbuf_;
         if (__extbuf_ == __extbuf_min_ && __rhs.__extbuf_ != __rhs.__extbuf_min_)
         {
----------------
ldionne wrote:
> I know nothing about this class' implementation and I haven't studied it deeply, but this is basically a small buffer optimization, right?
> 
> I'd add comments like this:
> 
> ```
> // *this uses the small buffer, __rhs doesn't
> if (...) {
> 
> }
> // *this doesn't use the small buffer, __rhs does
> else if (...) {
> 
> }
> // *this and __rhs both use the small buffer
> else {
> 
> }
> ```
> 
> Actually, this makes me think -- aren't we missing the case where neither is using the small buffer?
> I know nothing about this class' implementation and I haven't studied it deeply, but this is basically a small buffer optimization, right?

Right. I've added comments as you suggested.

> Actually, this makes me think -- aren't we missing the case where neither is using the small buffer?

Good question, but this case is already handled by the outer `if`. I've added a comment there as well.


================
Comment at: libcxx/include/fstream:452
+            char __tmp[sizeof(__extbuf_min_)];
+            _VSTD::memmove(__tmp, __extbuf_min_, sizeof(__extbuf_min_));
+            _VSTD::memmove(__extbuf_min_, __rhs.__extbuf_min_, sizeof(__extbuf_min_));
----------------
ldionne wrote:
> We can use `std::` for new code, no need to use `_VSTD::` anymore!
OK, thanks for the info. I was just "doing as the Romans do"...


================
Comment at: libcxx/test/std/input.output/file.streams/fstreams/filebuf.assign/nonmember_swap_min.pass.cpp:1
+//===----------------------------------------------------------------------===//
+//
----------------
ldionne wrote:
> This test seems to have failed on Windows: https://buildkite.com/llvm-project/libcxx-ci/builds/9676#30ce63ab-bc21-4a52-b236-4148667ef48c
Hm, I don't have a Windows machine where I could debug this, so I unfortunately can't fix this myself. But note that the assertion //before// the swap fails, so my changes can't have broken it, I think? I've disabled the test on Windows for now.


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

https://reviews.llvm.org/D122257