[libcxx-commits] [PATCH] D82490: [libcxx] Put clang::trivial_abi on std::unique_ptr

[Vy Nguyen via Phabricator via libcxx-commits]

oontvoo added inline comments.


================
Comment at: libcxx/docs/DesignDocs/UniquePtrTrivialAbi.rst:132
+
+ - Lifetime of local `returned` ``std::unique_ptr<>`` ends earlier.
+
----------------
ldionne wrote:
> Are you missing backticks around `returned`?
 wrong quotes. (meant to be emphasis, not code)


================
Comment at: libcxx/include/__config:109
+// Enable clang::trivial_abi on std::unique_ptr.
+#  ifdef _LIBCPP_ABI_ENABLE_UNIQUE_PTR_WITH_TRIVIAL_ABI
+#    define _LIBCPP_ABI_UNIQUE_PTR_TRIVIAL __attribute__((trivial_abi))
----------------
ldionne wrote:
> I would prefer if that `#define` was just a straight macro definition without any expansion. That would be more consistent with how we deal with other ABI macros.
> 
> What you could then do is say in `unique_ptr`:
> 
> ```
> template <class _Tp, class _Dp = default_delete<_Tp> >
> class 
> #if defined(_LIBCPP_ABI_UNIQUE_PTR_TRIVIAL)
>     __attribute__((trivial_abi))
> #endif
> _LIBCPP_TEMPLATE_VIS unique_ptr {
> ...
> ```
> 
> Or something equivalent. But ideally, the knob for turning on that optimization should just be to `#define` a macro to nothing.
I'd put this here because I thought we might need to add a check for availability of the attribute. probably easier to read than repeating the same logic in a few places

Re : knob on this, we can still just undef the `*ENABLE*` flag, yes? (Or re-write it to be bool)

But having no familiarity with how these macros are handled, I've no preference. 
Happy to change it, unless someone wants to chime in


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82490