EricWF requested changes to this revision.
EricWF added a reviewer: EricWF.
EricWF added a comment.
This revision now requires changes to proceed.
Added a bunch of inline comments.
The biggest requested change is removing the `__atomic_support` header. We only
need one atomic call within the headers. It's overkill to add a new header.
================
Comment at: libcxx/include/__atomic_support:1
+//===----------------------------------------------------------------------===////
+//
----------------
I would greatly prefer if this patch didn't add another header, and simply
defined `__libcpp_atomic_increment` and `__libcpp_atomic_decrement` in place of
`__atomic_inc_dec::increment`/`__atomic_inc_dec::decrement`.
================
Comment at: libcxx/include/memory:3802
+{
+ return __libcpp_atomic_add(&t, 1, _AO_Relaxed);
+}
----------------
sebpop wrote:
> EricWF wrote:
> > Why add `increment` and `decrement` at all? Just manually inline
> > `__libcpp_atomic_add` at the callsites.
> I like the idea to manually inline the inc and dec functions.
> What should we do with the NOTE: above?
>
> // NOTE: Relaxed and acq/rel atomics (for increment and decrement
> respectively)
> // should be sufficient for thread safety.
> // See https://llvm.org/bugs/show_bug.cgi?id=22803
>
> should we just go ahead and remove the note, or you want to have it where
> inc/dec are called? (about a dozen places.)
Neremind about the manually inlining bit.
Please remove the `__atomic_inc_dec` namespace and rename `increment` to
`__libcpp_atomic_increment` and `decrement` to `__libcpp_atomic_decrement`.
Please also remove the `__atomic_support` header and instead simply call
`__atomic_add_fetch` from inside the functions.
================
Comment at: libcxx/include/memory:3911
+#ifdef _LIBCPP_BUILDING_MEMORY
void __add_shared() _NOEXCEPT;
bool __release_shared() _NOEXCEPT;
----------------
Please apply `_LIBCPP_FUNC_VIS` to both of these methods.
================
Comment at: libcxx/include/memory:3914
+#else
+ inline _LIBCPP_INLINE_VISIBILITY
+ void __add_shared() _NOEXCEPT {
----------------
the `inline` in redundant if you define the function inside the class.
================
Comment at: libcxx/include/memory:3948
+#ifdef _LIBCPP_BUILDING_MEMORY
void __add_shared() _NOEXCEPT;
void __add_weak() _NOEXCEPT;
----------------
Please add `_LIBCPP_FUNC_VIS` to the three methods.
================
Comment at: libcxx/include/memory:3952
+#else
+ inline _LIBCPP_INLINE_VISIBILITY
+ void __add_shared() _NOEXCEPT {
----------------
`inline` is redundant here.
https://reviews.llvm.org/D24991
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
