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

[Zoe Carver via Phabricator via libcxx-commits]

zoecarver marked 3 inline comments as done.
zoecarver added inline comments.


================
Comment at: libcxx/include/syncstream:201
+
+    static _VSTD::unordered_map<streambuf_type*, mutex_ptr_type>  __mtx_ptr_map;
+};
----------------
mclow.lists wrote:
> As a static member variable - when does this get initialized? On first use? before `main`? Does it get initialized if the user never instantiates a `basic_syncbuf`? 
Yes, because it is initialized out of line, it will be initialized any time this file is included. 

I will update this to use an enum and get around the static data member initialization. 


================
Comment at: libcxx/include/syncstream:220
+{
+    mutex_ptr_type __mtx = __mtx_ptr_map[__obuf];
+
----------------
mclow.lists wrote:
> How do entries get removed from `__mtx_ptr_map`?
> 
They don't. I don't think there is any way to say "will any threads ever need to access this streambuf again?" 

If there was a way to only keep the streambuf in the static map while the object is active, I would be open to that. But, I am hesitant to do that because I want to keep the read/writes to the map as low as possible. 

What would you suggest? 


================
Comment at: libcxx/include/syncstream:279
+    bool __result = true;
+    mutex_ptr_type __mtx = __mtx_ptr_map[__wrapped];
+
----------------
mclow.lists wrote:
> This adds a default constructed `mutex_ptr_type` to the map if it doesn't already exist.
> I suspect that's not what you wanted.
I can update this to use `contains` but, I don't think it matters much. There should never be a scenario where `__mtx_ptr_map[__wrapped]` doesn't exist. And if there is, then the below code will still pass because `mutex_ptr_type == nullptr_t` is OK.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67086