bcraig added inline comments.
================
Comment at: include/__threading_support:201
@@ +200,3 @@
+// Mutex
+#define _LIBCPP_MUTEX_INITIALIZER nullptr
+struct __libcpp_platform_mutex_t;
----------------
rmaprath wrote:
> rmaprath wrote:
> > bcraig wrote:
> > > rmaprath wrote:
> > > > bcraig wrote:
> > > > > rmaprath wrote:
> > > > > > bcraig wrote:
> > > > > > > I'm not sure I like taking the freedom to define
> > > > > > > _LIBCPP_MUTEX_INITIALIZER away from implementers.
> > > > > > >
> > > > > > > Would it be too terrible to replace this entire #elif block with
> > > > > > > something like the following?
> > > > > > >
> > > > > > > ```
> > > > > > > #if !defined(__has_include) ||
> > > > > > > __has_include(<os_provided_thread.h>)
> > > > > > > #include <os_provided_thread.h>
> > > > > > > #else
> > > > > > > #error "_LIBCPP_THREAD_API_EXTERNAL requires the implementer to
> > > > > > > provide <os_provided_thread.h> in the include path"
> > > > > > > #endif
> > > > > > > ```
> > > > > > >
> > > > > > >
> > > > > > The problem is that, `std::mutex` constructor needs to be
> > > > > > `constexpr` (as you pointed out earlier). And since
> > > > > > `__libcpp_mutex_t` is a pointer type (for this externally threaded
> > > > > > variant), sensible definitions for `_LIBCPP_MUTEX_INITIALIZER` are
> > > > > > limited.
> > > > > >
> > > > > > Other than `nullptr`, one may be able to define
> > > > > > `_LIBCPP_MUTEX_INITIALIZER` to be a pointer to some constant mutex
> > > > > > (presuming that make it `constexpr` OK?) but I'm not convinced if
> > > > > > such a setup would be very useful.
> > > > > >
> > > > > > Hope that sounds sensible?
> > > > > If the implementer gets to provide an entire header, then they also
> > > > > get to choose what libcpp_mutex_t will be. They could make it a
> > > > > struct.
> > > > This externally-threaded library variant needs to be compiled against a
> > > > set API, so we have this header with declarations like
> > > > `__libcpp_mutex_lock()` which are referred from within the library
> > > > sources (same function signatures as those used in
> > > > `LIBCPP_THREAD_API_PTHREAD` - this allows us to keep the library source
> > > > changes to a minimum).
> > > >
> > > > Now, in this particular library variant, we want to differ these calls
> > > > to runtime rather than libcxx compile-time!
> > > >
> > > > On the other hand, I think you are proposing a compile-time (static)
> > > > thread porting setup where the platform vendors need to supply a header
> > > > file with appropriate definitions for these functions / types, and they
> > > > would compile libcxx themselves? That sounds like a good idea, but the
> > > > current patch is aiming for a different goal: we provide a pre-compiled
> > > > libcxx library which calls out to those `__libcpp_xxx` functions at
> > > > runtime, and platform vendors need to provide those functions, they
> > > > don't have to compile libcxx themselves. This is what forces us to use
> > > > opaque pointer types to capture platform-defined threading primitives.
> > > >
> > > > Perhaps I could improve the naming here, may be
> > > > `LIBCPP_HAS_RUNTIME_THREAD_API`? Or `LIBCPP_HAS_DYNAMIC_THREAD_API`?
> > > That is an excellent summary, and it does a good job of explaining the
> > > disconnect we were having. I'm going to push forward with the disconnect
> > > though :)
> > >
> > > I think you can implement the dynamic case in terms of the static case
> > > without loss of generality or performance. You ("Vendor X") could
> > > pre-compile your library with a pointer-sized libcpp_mutex_t defined in
> > > <os_provided_thread.h>, and at runtime call out to a different library
> > > that has an implementation of libcpp_mutex_lock. Someone else ("Vendor
> > > Y") could have a larger libcpp_mutex_t defined, and provide a static
> > > inline definition of libcpp_mutex_lock.
> > OK, I think I understand your point :)
> >
> > Will meditate on it a bit and spin a new patch tomorrow. Thanks!
> >
> >
> There is a slight complication here. If we fully offload the external
> thread-API to platform vendors, nobody will be able to build this
> externally-threaded library variant using the vanilla libcxx sources, because
> they would have to first come up with a `platform_threads.h` header.
>
> We could provide a `dynamic_threads.h` header with the method signatures only
> (which is selected if the `platform_threads.h` header is absent - and you get
> a "dynamically-threaded" library build). Now the library can be built +
> tested with the vanilla upstream sources. But note that we are introducing
> yet another header. I remember @mclow.lists mentioned that each additional
> header adds to the overhead of header lookup and we should try to keep that
> to a minimum.
>
> We can workaround this additional header by not collecting it when building
> the normal library variant (so it doesn't add to the header lookup overhead).
>
> Does that sound like an OK approach?
Sounds reasonable.
http://reviews.llvm.org/D20328
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
