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

[Louis Dionne via Phabricator via libcxx-commits]

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

Thanks for looking into this! I have some comments, but overall this looks sensible. I don't know this implementation very well, apologies if some of my comments/questions are naive.



================
Comment at: libcxx/include/fstream:428-436
+        ptrdiff_t __ln = 0, __le = 0, __rn = 0, __re = 0;
+        if (__extbufnext_ != nullptr)
+            __ln = __extbufnext_ - __extbuf_;
+        if (__extbufend_ != nullptr)
+            __le = __extbufend_ - __extbuf_;
+        if (__rhs.__extbufnext_ != nullptr)
+            __rn = __rhs.__extbufnext_ - __rhs.__extbuf_;
----------------
Suggestion (IMO that makes it clearer because we can see a pattern in how we initialize those):

```
ptrdiff_t __ln = __extbufnext_       ? __extbufnext_ - __extbuf_             : 0;
ptrdiff_t __le = __extbufend_        ? __extbufend_ - __extbuf_              : 0;
ptrdiff_t __rn = __rhs.__extbufnext_ ? __rhs.__extbufnext_ - __rhs.__extbuf_ : 0;
ptrdiff_t __re = __rhs.__extbufend_  ? __rhs.__extbufend_ - __rhs.__extbuf_  : 0;
```



================
Comment at: libcxx/include/fstream:437
+            __re = __rhs.__extbufend_ - __rhs.__extbuf_;
         if (__extbuf_ == __extbuf_min_ && __rhs.__extbuf_ != __rhs.__extbuf_min_)
         {
----------------
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?


================
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_));
----------------
We can use `std::` for new code, no need to use `_VSTD::` anymore!


================
Comment at: libcxx/test/std/input.output/file.streams/fstreams/filebuf.assign/nonmember_swap_min.pass.cpp:1
+//===----------------------------------------------------------------------===//
+//
----------------
This test seems to have failed on Windows: https://buildkite.com/llvm-project/libcxx-ci/builds/9676#30ce63ab-bc21-4a52-b236-4148667ef48c


================
Comment at: libcxx/test/std/input.output/file.streams/fstreams/filebuf.assign/nonmember_swap_min.pass.cpp:11
+
+// template <class charT, class traits = char_traits<charT> >
+// class basic_filebuf
----------------
Can you add a comment explaining what this tests, along with links to the fixed issues?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122257