[PATCH] D78897: [Support] raw_fd_ostream can lock file before write

[Serge Pavlov via Phabricator via llvm-commits]

sepavloff added a comment.

In D78897#2007992 <https://reviews.llvm.org/D78897#2007992>, @labath wrote:

> In D78897#2007851 <https://reviews.llvm.org/D78897#2007851>, @sepavloff wrote:
>
> > In D78897#2005085 <https://reviews.llvm.org/D78897#2005085>, @labath wrote:
> >
> > > Overall I feel that this sort of automatic locking will very rarely be the "right tool for the job". I think your follow-up patch sort of demonstrates that, because you've needed to format the output to a temporary string stream in order for it do what you need. I think it might be more reasonable to just leave this out of the raw_ostream class, and do a manual `lockFile` + `write` + `unlockFile` combo where needed.
> >
> >
> > Of course, this is a matter of convenience. Automatic locking, as implemented in this patch is not flexible enough, it must be improved. But manual operations might be error-prone.
>
>
> Well.. the thing is, I am not sure automatic locking *can* be improved to be useful. I find this situation very reminiscent of various "thread-safe" containers. While in theory we could have an `std::thread_safe_map`, I think there's a (good) reason that the standard library does not provide it -- it usually is not that useful, because one needs to have more control over the scope of the "critical sections". A typical use case for a thread-safe map is to compute something once and then cache it, but there one needs the whole `test-and-set` block to be atomic, not just individual operations.
>
> I think this is fairly similar to what happens with raw_ostream, where one typically would wants to have locking granularity be different from what happens to fall out of the stream implementation. And in that case, I am not sure the implementation needs to be inside the stream class. Maybe an RAII object is in order ?
>
>   {
>     stream_locker Lock(OS); // not sure what to do about error handling
>     OS << whatever;
>   } // Lock destructor calls OS.flush(), and unlocks ?
>


Exactly!
It is in D79066 <https://reviews.llvm.org/D79066>.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78897