[libcxx-commits] [PATCH] D151223: [libc++] Fixes use-after move diagnostic.

[Mark de Wever via Phabricator via libcxx-commits]

Mordante marked 4 inline comments as done.
Mordante added inline comments.


================
Comment at: libcxx/include/strstream:261
         : istream(_VSTD::move(__rhs)),
-          __sb_(_VSTD::move(__rhs.__sb_))
+          __sb_(_VSTD::move(__rhs.__sb_)) // NOLINT(bugprone-use-after-move)
     {
----------------
philnik wrote:
> ldionne wrote:
> > Mordante wrote:
> > > ldionne wrote:
> > > > What is the non-standard extension? I don't understand why this code is valid and this isn't a bug? Does `istream(_VSTD::move(__rhs))` above not move the `__sb_` member? If not, would it be possible to instead do something like
> > > > 
> > > > ```
> > > > istream(std::move(static_cast<istream&>(__rhs)))
> > > > ```
> > > > 
> > > > to make it clear that we're only moving `__rhs` as a base class object (if that's what's going on)?
> > > The non-Standard part are the move operations. I've updated the description of the patch.
> > > 
> > > I can make that change, it looks closer to my original edit `istream(static_cast<istream&&>(__rhs))`, but this is less subtle :-)
> > Yeah I think that makes it a bit more obvious. We might not even need to add a `NOLINT` annotation then?
> TBH the `std::move` makes more sense to me than the `static_cast`, which IMO would warrant a comment.
I've switched to Louis' suggestion only kept the `_VSTD` to keep the patch minimal.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151223