EricWF added a comment.
In https://reviews.llvm.org/D24991#565715, @mclow.lists wrote:
> How does this play with existing binaries? Applications that expect these
> functions to exist in the dylib?
This patch is majorly ABI breaking, although we could probably find a
formulation that wasn't.
@hxy9243 wrote:
> which gives potential optimization opportunities and performance benefits.
Please provide benchmark tests which demonstrate that these benefits are
concrete and not just "potential". Moving methods out of the dylib is no easy
task so I would like hard evidence that it's worth while.
================
Comment at: libcxx/include/memory:3793
+namespace
+{
----------------
Anonymous namespaces are a C++11 feature and this is C++03 code.
================
Comment at: libcxx/include/memory:3798
+// See https://llvm.org/bugs/show_bug.cgi?id=22803
+template <class T> __attribute__((always_inline))
+inline _LIBCPP_INLINE_VISIBILITY T
----------------
* `T` and `increment` need to be reserved names.
* Never use `__attribute__((always_inline))` directly, that's why we have
visibility macros.
================
Comment at: libcxx/include/memory:3802
+{
+ return __libcpp_atomic_add(&t, 1, _AO_Relaxed);
+}
----------------
Why add `increment` and `decrement` at all? Just manually inline
`__libcpp_atomic_add` at the callsites.
================
Comment at: libcxx/include/memory:3818
virtual ~bad_weak_ptr() _NOEXCEPT;
- virtual const char* what() const _NOEXCEPT;
+ virtual const char* what() const _NOEXCEPT {
+ return "bad_weak_ptr";
----------------
Why would you want to inline this?
Repository:
rL LLVM
https://reviews.llvm.org/D24991
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
