[libcxx-commits] [PATCH] D67086: Implement syncstream (p0053)

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added inline comments.


================
Comment at: libcxx/include/syncstream:157
+#  ifndef _LIBCPP_HAS_NO_THREADS
+class __wrapped_streambuf_mutex {
+  _LIBCPP_HIDE_FROM_ABI __wrapped_streambuf_mutex() = default;
----------------
Mordante wrote:
> ldionne wrote:
> > This seems to be a generic utility to lock addresses. We could extract it from `<syncstream>` and make it generic, and then reuse it from here in `<syncstream>`. That way we could potentially reuse this code if we have a need for it in the future.
> > 
> > Or we could also maybe even abstract away the fact that we're containing a `mutex` inside the `value_type`. Then this would be a generic map from addresses to some payload.
> I prefer to do this in a followup commit. I feel the commit is already quite large and I prefer generic tools not to be hidden in these commits.
Can we file a Github issue and add a TODO here to look into this? That way, we have something actionable to follow up on in a PR in the near future after shipping this patch.


================
Comment at: libcxx/include/syncstream:386
+
+  basic_string<_CharT, _Traits, _Allocator> __str_;
+  bool __emit_on_sync_{false};
----------------
Mordante wrote:
> Mordante wrote:
> > ldionne wrote:
> > > Do we take advantage of what `std::string` provides (i.e. growth characteristics, SBO, etc..). If we do take advantage of those, then it might be worth using `std::string`, otherwise I think we might as well use a character buffer we manually manage to save on the duplication of begin/end information.
> > No basically the SBO is "killed" by "__str_.resize(__str_.capacity() + 1);" This could be smarter and keep the SBO. I'm not sure how useful SBO is. It only helps if all data flushed fits in the SBO area.
> I prefer this too for a separate review. Especially since I already have something not generic in the implementation of format. That could benefit from a generic routine. The same for the places that use the now deprecated `get_temporary_buffer`. That would allow us to properly deprecate that function.
This review is extremely relevant for any attempt to remove `temporary_buffer` again: https://reviews.llvm.org/D152208

FWIW I think we should 100% do it and we meant to pick this up again but it hasn't happened yet.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67086