[libcxx-commits] [PATCH] D89057: Add the C++17 <memory_resource> header (mono-patch)
Louis Dionne via Phabricator via libcxx-commits
libcxx-commits at lists.llvm.org
Tue Feb 8 15:55:26 PST 2022
ldionne requested changes to this revision.
ldionne added inline comments.
This revision now requires changes to proceed.
================
Comment at: libcxx/docs/Status/Cxx20Issues.csv:236
"`3307 <https://wg21.link/LWG3307>`__","``std::allocator<void>().allocate(n)``\ ","Prague","",""
-"`3310 <https://wg21.link/LWG3310>`__","Replace ``SIZE_MAX``\ with ``numeric_limits<size_t>::max()``\ ","Prague","",""
+"`3310 <https://wg21.link/LWG3310>`__","Replace ``SIZE_MAX``\ with ``numeric_limits<size_t>::max()``\ ","Prague","|Complete|","14.0"
"`3313 <https://wg21.link/LWG3313>`__","``join_view::iterator::operator--``\ is incorrectly constrained","Prague","","","|ranges|"
----------------
This should be 15.0 since we slipped again.
================
Comment at: libcxx/docs/Status/Cxx2bIssues.csv:19
"`3170 <https://wg21.link/LWG3170>`__","``is_always_equal`` added to ``std::allocator`` makes the standard library treat derived types as always equal","November 2020","",""
-"`3036 <https://wg21.link/LWG3036>`__","``polymorphic_allocator::destroy`` is extraneous","November 2020","",""
+"`3036 <https://wg21.link/LWG3036>`__","``polymorphic_allocator::destroy`` is extraneous","November 2020","|Complete|","14.0"
"`3171 <https://wg21.link/LWG3171>`__","LWG2989 breaks ``directory_entry`` stream insertion","November 2020","",""
----------------
Same, 15.0.
================
Comment at: libcxx/include/__memory_resource/polymorphic_allocator.h:65
+ _ValueType *allocate(size_t __n) {
+ if (__n > (size_t(-1) / sizeof(_ValueType))) {
+ __throw_bad_array_new_length();
----------------
Please use `numeric_limits<size_t>::max()`. I know they are equivalent, but being close to the Standard is good. I don't mind taking an additional dependency on `<limits>` at all. This applies elsewhere too.
================
Comment at: libcxx/include/__memory_resource/polymorphic_allocator.h:153-155
+ _LIBCPP_HIDE_FROM_ABI
+ memory_resource *resource() const noexcept
+ { return __res_; }
----------------
This is nitpicky on a style matter, but can you please just do
```
memory_resource *resource() const noexcept {
return __res_;
}
```
or
```
memory_resource *resource() const noexcept { return __res_; }
```
if you really care about saving a line. This applies elsewhere too.
The reason I'm asking this is that we don't use the pattern you have here anywhere else (that I can tell), and I don't want to introduce yet another stylistic inconsistency.
================
Comment at: libcxx/include/__memory_resource/synchronized_pool_resource.h:18
+#if !defined(_LIBCPP_HAS_NO_THREADS)
+#include <mutex>
+#endif
----------------
Please indent includes when they are nested to near-located `#if`s.
================
Comment at: libcxx/include/__memory_resource/unsynchronized_pool_resource.h:105-109
+ memory_resource *__res_;
+ __adhoc_pool __adhoc_pool_;
+ __fixed_pool *__fixed_pools_;
+ int __num_fixed_pools_;
+ uint32_t __options_max_blocks_per_chunk_;
----------------
It sucks that we have to commit to an ABI right here. It would be great if we could instead store a pimpl, but that would require allocating and that's not a great idea.
================
Comment at: libcxx/include/memory_resource:2
+// -*- C++ -*-
+//===------------------------ memory_resource -----------------------------===//
+//
----------------
We don't include the name of the file anymore!
================
Comment at: libcxx/include/memory_resource:20
+
+ class memory_resource;
+
----------------
Can we get section names like `// [mem.res.class]`?
================
Comment at: libcxx/include/regex:6801
+#if _LIBCPP_STD_VER > 14
+_LIBCPP_BEGIN_NAMESPACE_STD
+namespace pmr
----------------
Any reason for not putting those declarations inside the `_LIBCPP_BEGIN_NAMESPACE_STD` that was closed just above?
================
Comment at: libcxx/include/regex:6805
+ template<class _BidirT>
+ using match_results = _VSTD::match_results<_BidirT, polymorphic_allocator<_VSTD::sub_match<_BidirT>>>;
+
----------------
Suggestion: you don't have to, but you could move all your new code to `std::` instead of `_VSTD::`.
================
Comment at: libcxx/include/string:4593
+ using u32string = basic_string<char32_t>;
+ using wstring = basic_string<wchar_t>;
+}
----------------
I think this needs to be guarded by `_LIBCPP_HAS_NO_WIDE_CHARACTERS`. I would expect this to be caught by the CI.
================
Comment at: libcxx/lib/abi/CHANGELOG.TXT:1
ABI Changelog
==============
----------------
[commenting here for lack of better place]
Another thing we'll need to add is availability macros for the newly added dylib functionality. You can look at `include/__availability` for how that's done.
================
Comment at: libcxx/lib/abi/CHANGELOG.TXT:111
+* [libc++] [C++17] Implement <memory_resource>.
+
----------------
This should be moved under a new section `15.0`. Sorry for the churn.
================
Comment at: libcxx/lib/abi/CHANGELOG.TXT:156
+ ------------------------
+ Symbol added: TODO FIXME BUG HACK
+
----------------
You can figure this one out by running `./libcxx/utils/ci/run-buildbot-container` locally (requires Docker) and then running the `generate-cxx-abilist` target on Linux.
================
Comment at: libcxx/src/memory_resource.cpp:1
+//===------------------------ memory_resource.cpp -------------------------===//
+//
----------------
No file name!
================
Comment at: libcxx/src/memory_resource.cpp:13
+
+#ifndef _LIBCPP_HAS_NO_ATOMIC_HEADER
+#include "atomic"
----------------
Please indent nested `#include`s.
================
Comment at: libcxx/src/memory_resource.cpp:28
+
+//memory_resource::~memory_resource() {}
+
----------------
Why not remove this altogether?
================
Comment at: libcxx/src/memory_resource.cpp:32
+
+#ifdef _LIBCPP_HAS_NO_ALIGNED_ALLOCATION
+static bool is_aligned_to(void *ptr, size_t align)
----------------
Let's always include this.
================
Comment at: libcxx/src/memory_resource.cpp:53
+ void *result = _VSTD::__libcpp_allocate(bytes, align);
+ if (!is_aligned_to(result, align)) {
+ _VSTD::__libcpp_deallocate(result, bytes, align);
----------------
So when aligned allocation is not supported, we allocate unaligned and try to align the pointer within the allocated buffer. If that works, we're happy, otherwise we fail. I don't understand why that would ever succeed, can you explain?
Also, I've always been a bit confused by the `_LIBCPP_HAS_NO_ALIGNED_ALLOCATION` situation. It would be worth looking into whether we can just assume that it's provided when we build the library.
================
Comment at: libcxx/src/memory_resource.cpp:111
+{
+#ifndef _LIBCPP_HAS_NO_ATOMIC_HEADER
+ _LIBCPP_SAFE_STATIC static atomic<memory_resource*> __res{&res_init.resources.new_delete_res};
----------------
Is there a reason why you don't just assume that atomics are available? I would do:
```
#if defined(_LIBCPP_HAS_NO_THREADS)
<no threading implementation>
#else
<use atomic>
#endif
```
================
Comment at: libcxx/src/memory_resource_init_helper.h:2
+#pragma GCC system_header
+_LIBCPP_SAFE_STATIC ResourceInitHelper res_init _LIBCPP_INIT_PRIORITY_MAX;
----------------
Once you rebase, you'll need to use `_LIBCPP_CONSTINIT`.
================
Comment at: libcxx/test/std/utilities/utility/mem.res/mem.poly.allocator.class/mem.poly.allocator.mem/resource.pass.cpp:10
+// UNSUPPORTED: c++98, c++03, c++11, c++14
+// XFAIL: use_system_cxx_lib && target={{.+}}-apple-macosx10.{{9|10|11|12|13|14|15}}
+
----------------
Here and everywhere else, you'll need to also add
```
// XFAIL: use_system_cxx_lib && target={{.+}}-apple-macosx{{11|12}}
```
================
Comment at: libcxx/test/support/test_std_memory_resource.h:12-20
+#include <memory>
+#include <memory_resource>
+#include <type_traits>
+#include <utility>
+#include <cstddef>
+#include <cstdlib>
+#include <cstring>
----------------
Sort?
================
Comment at: libcxx/utils/generate_feature_test_macro_components.py:460
}, {
"name": "__cpp_lib_math_special_functions",
"values": { "c++17": 201603 },
----------------
Can we add a release note?
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