[libcxx-commits] [PATCH] D89057: Add the C++17 <memory_resource> header (mono-patch)
Arthur O'Dwyer via Phabricator via libcxx-commits
libcxx-commits at lists.llvm.org
Fri Jan 7 12:07:10 PST 2022
Quuxplusone added inline comments.
================
Comment at: libcxx/include/__memory_resource/memory_resource.h:29
+{
+ static const size_t __max_align = alignof(max_align_t);
+
----------------
philnik wrote:
> Is there a reason this isn't constexpr or just directly in lines 35 and 39?
The answer in both cases is that this came via copy-paste from `<experimental/memory_resource>`. I agree s/const/constexpr/g is an improvement. I'll keep it pulled out as a named constant, though, since it's used in two places and it's just barely conceivable that some platform might like to change it.
================
Comment at: libcxx/include/__memory_resource/memory_resource.h:39
+ _LIBCPP_HIDE_FROM_ABI
+ void deallocate(void *__p, size_t __bytes, size_t __align = __max_align)
+ { do_deallocate(__p, __bytes, __align); }
----------------
philnik wrote:
> Nit: Use `void*` instead of `void *` or at least do it consistently (see line 35).
This is one of those places where I think LLVM style is `T* x` but my-preference and at-least-half-the-time-libc++-style is `T *x`. I'll change line 35 instead.
================
Comment at: libcxx/include/__memory_resource/synchronized_pool_resource.h:61
+#if !defined(_LIBCPP_HAS_NO_THREADS)
+ unique_lock<mutex> __lk(__mut_);
+#endif
----------------
philnik wrote:
> Remember https://youtu.be/F6Ipn7gCOsY?t=1835?
:) I actually have no idea why I did this here. Will fix.
================
Comment at: libcxx/include/__memory_resource/unsynchronized_pool_resource.h:96
+protected:
+ void *do_allocate(size_t __bytes, size_t __align) override; // key function
+
----------------
philnik wrote:
> What do you want to say with `// key function`? For the vtable?
Yes, that's right; see also
```
../libcxx/include/__filesystem/filesystem_error.h: ~filesystem_error() override; // key function
```
================
Comment at: libcxx/include/deque:3027-3035
+#if _LIBCPP_STD_VER > 14
+_LIBCPP_BEGIN_NAMESPACE_STD
+namespace pmr
+{
+ template <class _ValueT>
+ using deque = _VSTD::deque<_ValueT, polymorphic_allocator<_ValueT>>;
+}
----------------
philnik wrote:
> Why is this not just in the above std namespace?
Here and also in the C++20 Ranges stuff, I've got a habit of "returning to zero" in between unrelated facilities, i.e. I'll nest things like `(x)((y))(x)` rather than `(x(y)x)`. IMO this makes it easier to keep track of the nesting levels. It's not to the level of a conscious guideline with me, necessarily — it's mostly muscle memory / personal style — but if you explicitly //ask// me to pick one style or the other, well, I'll still pick this one.
Leaving as-is, at least until a consensus builds against it. ;)
================
Comment at: libcxx/lib/abi/CHANGELOG.TXT:26-60
+ Symbol added: __ZNKSt3__13pmr26synchronized_pool_resource11do_is_equalERKNS0_15memory_resourceE
+ Symbol added: __ZNKSt3__13pmr28unsynchronized_pool_resource12__pool_indexEmm
+ Symbol added: __ZNKSt3__13pmr28unsynchronized_pool_resource17__pool_block_sizeEi
+ Symbol added: __ZNKSt3__13pmr28unsynchronized_pool_resource22__log2_pool_block_sizeEi
+ Symbol added: __ZNKSt3__13pmr28unsynchronized_pool_resource7optionsEv
+ Symbol added: __ZNSt3__13pmr19new_delete_resourceEv
+ Symbol added: __ZNSt3__13pmr20get_default_resourceEv
----------------
philnik wrote:
> Are these symbols not added under linux?
They are; I just was too lazy to dig up the build artifact that lists them. Will fix after a build completes again.
================
Comment at: libcxx/src/memory_resource.cpp:17
+#if defined(__ELF__) && defined(_LIBCPP_LINK_PTHREAD_LIB)
+#pragma comment(lib, "pthread")
+#endif
----------------
philnik wrote:
> Doesn't this only work with lld?
Copied from `experimental/memory_resource.cpp`, so it'll work as well as that one does (for better and worse).
================
Comment at: libcxx/src/memory_resource.cpp:126-135
+ if (set) {
+ new_res = new_res ? new_res : new_delete_resource();
+ lock_guard<mutex> guard(res_lock);
+ memory_resource *old_res = res;
+ res = new_res;
+ return old_res;
+ } else {
----------------
philnik wrote:
> And with the other `set` checks as well.
This was originally copy-pasted from `src/experimental/`... but the early-return rewrite strikes me as harder to read, so I'm keeping it as-is for now.
================
Comment at: libcxx/src/memory_resource.cpp:127
+ if (set) {
+ new_res = new_res ? new_res : new_delete_resource();
+ lock_guard<mutex> guard(res_lock);
----------------
philnik wrote:
>
PSA: You flipped the sense of this; you meant `!= nullptr`. Fixed.
================
Comment at: libcxx/src/memory_resource.cpp:212
+{
+ _LIBCPP_ASSERT(__first_ != nullptr, "deallocating a block that was not allocated with this allocator");
+ if (__first_->__start_ == p) {
----------------
philnik wrote:
> Wouldn't it make more sense to put that in the header so the dylib doesn't have to change depending on wanting these assertions or not?
I agree with your goal, but this guy's caller is `unsynchronized_pool_resource::do_deallocate` (also in `src/`), so I don't think it's possible. Not without leaking a lot more stuff into the headers, which conflicts with our other goal of trying to maintain some semblance of ability-to-change-implementation.
================
Comment at: libcxx/src/memory_resource.cpp:426-427
+ if (result == nullptr) {
+ auto min = [](size_t a, size_t b) { return a < b ? a : b; };
+ auto max = [](size_t a, size_t b) { return a < b ? b : a; };
+
----------------
philnik wrote:
> Why not use std::min and std::max?
Originally, either because `<algorithm>` had not been granularized and I didn't want to drag in all of `<algorithm>` for this (although that's a bad rationale), or because these can deal with mixed types (e.g. `min(sizeof(int), 4)` whereas `_VSTD::min` would require casting.
But now: sure, OK.
================
Comment at: libcxx/src/memory_resource.cpp:448-451
+ min(
+ __max_blocks_per_chunk,
+ __options_max_blocks_per_chunk_
+ )
----------------
FYI, this is the call that requires a cast after switching to `_VSTD::min`.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D89057/new/
https://reviews.llvm.org/D89057
More information about the libcxx-commits
mailing list