This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Fixes use-after move diagnostic.
ClosedPublic

Authored by Mordante on May 23 2023, 8:15 AM.

Details

Reviewers
ldionne
Group Reviewers
Restricted Project
Commits
rGac7d60f73a4a: [libc++] Fixes use-after move diagnostic.
Summary

The diagnostic is issued by clang-tidy 17.

This just suppressed the diagnostic. The move operations are non-standard extensions and the class itself is deprecated.

Diff Detail

Event Timeline

Mordante created this revision.May 23 2023, 8:15 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 23 2023, 8:15 AM
Mordante updated this revision to Diff 525705.May 25 2023, 11:01 AM

Rebased and a different approach.

Mordante updated this revision to Diff 526065.May 26 2023, 8:02 AM

Rebased to fix the CI.

Mordante published this revision for review.May 26 2023, 11:42 AM
Mordante edited the summary of this revision. (Show Details)
Herald added a project: Restricted Project. · View Herald TranscriptMay 26 2023, 11:42 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
ldionne added inline comments.
libcxx/include/strstream
261

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)?

Mordante edited the summary of this revision. (Show Details)May 30 2023, 8:21 AM
Mordante added inline comments.
libcxx/include/strstream
261

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 :-)

ldionne added inline comments.May 30 2023, 8:52 AM
libcxx/include/strstream
258–259

Here and below.

261

Yeah I think that makes it a bit more obvious. We might not even need to add a NOLINT annotation then?

ldionne requested changes to this revision.May 30 2023, 8:54 AM
This revision now requires changes to proceed.May 30 2023, 8:54 AM

I still don't understand what extension is supposed to be used. Not a single compiler complains about casting an rvalue to a base: https://godbolt.org/z/bv4Moh5on

libcxx/include/strstream
261

TBH the std::move makes more sense to me than the static_cast, which IMO would warrant a comment.

Mordante updated this revision to Diff 526679.May 30 2023, 9:43 AM

Addresses review comments.

Mordante marked 4 inline comments as done.May 30 2023, 9:45 AM
Mordante added inline comments.
libcxx/include/strstream
261

I've switched to Louis' suggestion only kept the _VSTD to keep the patch minimal.

ldionne added inline comments.May 30 2023, 2:01 PM
libcxx/include/strstream
260

Don't you mean istream& here instead? You want to cast to the base class?

309

Same here.

369

This one looks right.

Mordante updated this revision to Diff 526909.May 30 2023, 10:49 PM
Mordante marked an inline comment as done.

Addresses review comments.

Mordante marked 2 inline comments as done.May 30 2023, 10:50 PM

Thanks for catching me copy-pasting from the wrong place.

ldionne accepted this revision.May 31 2023, 7:56 AM
This revision is now accepted and ready to land.May 31 2023, 7:56 AM
This revision was automatically updated to reflect the committed changes.