[libcxx-commits] [PATCH] D154017: Cleanup __uninitialized_temporary_buffer internals.

[Eric Fiselier via Phabricator via libcxx-commits]

EricWF marked an inline comment as done.
EricWF added inline comments.


================
Comment at: libcxx/include/__algorithm/inplace_merge.h:228
     difference_type __buf_size = _VSTD::min(__len1, __len2);
-    auto __buf = std::__make_uninitialized_buffer<value_type[]>(nothrow, __buf_size);
     return std::__inplace_merge<_AlgPolicy>(
----------------
ldionne wrote:
> We were using `nothrow` to make it clear that this will return `nullptr` instead of throwing if the allocation fails. This was initially designed in the context of making sure that exception safety is really easy to determine since this will be used inside PSTL, where we want to have strict control over what can and can't throw (because we need to exclusively propagate exceptions that originate from the library "setup" code, but terminate for any other user-thrown exception).
> 
> I still feel like `nothrow` has its place here in that context.
I strongly disagree. It's very confusing to introduce a novel use of a tag type, to document rather than dispatch, on an API that's purpose is to _maybe_ provide memory _if_ it's available. 

If we're looking to document the semantics of `get_temporary_buffer` further, that's OK. If we're looking to improve the usability & safety of the of the API, that's OK too.
But let's proceed with that some other way.

I'll change the name to get `__maybe_get_temporary_buffer`. 



================
Comment at: libcxx/include/__memory/uninitialized_buffer.h:33
+  _LIBCPP_HIDE_FROM_ABI
+  explicit _LIBCPP_CONSTEXPR  __uninitialized_buffer_deleter(size_t __count = 0) _NOEXCEPT
+      : __count_(__count) {}
----------------
ldionne wrote:
> I don't think we need the default argument here since you're always passing an argument to the constructor below.
I'm with you. I tried to make this not have a default constructor, but there are a bunch of uses in the algorithms header where the type is default constructod.




================
Comment at: libcxx/include/new:297
 
+inline _LIBCPP_INLINE_VISIBILITY
+void *__libcpp_allocate(size_t __size, size_t __align, nothrow_t) {
----------------
Mordante wrote:
> s/_LIBCPP_INLINE_VISIBILITY/_LIBCPP_HIDE_FROM_ABI/
Do we have a doc describing what the difference is and providing top? That way I can stop making the mistake in the future?


================
Comment at: libcxx/include/new:300
+#ifndef _LIBCPP_HAS_NO_ALIGNED_ALLOCATION
+  if (__is_overaligned_for_new(__align)) {
+    const align_val_t __align_val = static_cast<align_val_t>(__align);
----------------
MaskRay wrote:
> Mordante wrote:
> > Mainly curious, but would it make sense to have this check in the Clang builtin?
> > Mainly curious, but would it make sense to have this check in the Clang builtin?
> 
> Which check?
I think they mean dispatching the call to overaligned new. And I don't think so.
The clang builtin is a very thin wrapper whose only job is to annotate the function call as "builtin" so it can be elided.

Plus, I think the library has more knowledge about the availability of these functions, since we provide them.


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

https://reviews.llvm.org/D154017