This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Add get_allocator and some constructor overloads of P0408R7 (Efficient Access to basic_stringbuf's Buffer)
AbandonedPublic

Authored by pfusik on Jun 21 2023, 1:34 AM.

Details

Reviewers
None
Group Reviewers
Restricted Project
Summary

Diff Detail

Event Timeline

pfusik created this revision.Jun 21 2023, 1:34 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 21 2023, 1:34 AM
pfusik requested review of this revision.Jun 21 2023, 1:34 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 21 2023, 1:34 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript

Sorry, but would it be possible to to a patch per class instead, starting with stringbuf? That way I can keep track that all classes are done. The subtile differences in all overloads make it really annoying the review for correctness.

pfusik added a comment.EditedJun 24 2023, 6:20 AM

Sorry, but would it be possible to to a patch per class instead, starting with stringbuf? That way I can keep track that all classes are done. The subtile differences in all overloads make it really annoying the review for correctness.

Would you like all the changes in stringbuf in one patch? That's quite a lot.
The remaining three classes are just simple wrappers over stringbuf.
I thought it would be easier to split into features.

Also, I expect that users don't typically use stringbuf directly, so having features exposed in *stringstream is what they care. That's how we proceeded with the already merged view().

Sorry, but would it be possible to to a patch per class instead, starting with stringbuf? That way I can keep track that all classes are done. The subtile differences in all overloads make it really annoying the review for correctness.

Would you like all the changes in stringbuf in one patch? That's quite a lot.
The remaining three classes are just simple wrappers over stringbuf.
I thought it would be easier to split into features.

Yes I would like that, I've already reviewed all code changes in stringbuf so that would be faster for me to review again.

Also, I expect that users don't typically use stringbuf directly, so having features exposed in *stringstream is what they care. That's how we proceeded with the already merged view().

I agree, that most users don't care about these "internals". Still we need them to implement the "externals". I think the externals are smaller and can be reviewed a lot faster. It's mainly that I have limited time to work on libc++ and large complex patches are quite hard to find time for.

pfusik abandoned this revision.Jun 24 2023, 9:04 AM

Ok, I'll submit a patch implementing the whole paper in stringbuf. Since you asked for a separate test per overload, be prepared for two dozen tests. :)