This is an archive of the discontinued LLVM Phabricator instance.

[libcxx] Use custom allocator's `construct` in C++03 when available.
ClosedPublic

Authored by vsapsai on Jun 28 2018, 5:21 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

vsapsai created this revision.Jun 28 2018, 5:21 PM

I'm pretty sure that the C++03 standard doesn't permit the implementation to call any construct method here, even if it wanted to. And this adds a couple hundred LOC to get that (non-standard) behavior. Is there a specific use-case that you're trying to enable by adding this behavior to libc++?

I'm pretty sure that the C++03 standard doesn't permit the implementation to call any construct method here, even if it wanted to. And this adds a couple hundred LOC to get that (non-standard) behavior. Is there a specific use-case that you're trying to enable by adding this behavior to libc++?

Only the standard behaviour is required, so I need only

template <class _Tp, class _A0>
       static void construct(allocator_type& __a, _Tp* __p, const _A0& __a0);

to call allocator's construct. Other construct methods with different number of parameters were changed only for completeness. Also I'm thinking that using available matching allocator methods is something that developers would expect. But I don't think this opinion should influence decisions because seems there is no much use for custom allocator_type::construct.

If nobody thinks all variadic cases should be supported, I'll clean that up.

vsapsai updated this revision to Diff 154018.Jul 3 2018, 4:42 PM
  • Clean up functionality not required by the Standard.
Quuxplusone added inline comments.Jul 3 2018, 9:09 PM
libcxx/test/std/containers/sequences/vector/vector.cons/construct_iter_iter_alloc.pass.cpp
60 ↗(On Diff #154018)

Per my comments on D48342, I think it would be fun to add a test allocator like

struct cpp03_fun_allocator : bare_allocator<T> {
    ...
    void construct(pointer p, const value_type& val) {
        construct(p, val, std::is_class<T>{});
    }
    void construct(pointer p, const value_type& val, std::true_type) {
        ::new(p) value_type(val);
    }
    void construct(pointer p, const value_type& val, std::false_type) {
        ::new(p) value_type(val);
    }

and just see whether it passes the test suite. If it does, might as well add that test. But if it doesn't, *I'm* not going to force anyone to fix it. Allocators in C++03 seems like masochism to me. :)

vsapsai added inline comments.Jul 4 2018, 4:16 PM
libcxx/test/std/containers/sequences/vector/vector.cons/construct_iter_iter_alloc.pass.cpp
60 ↗(On Diff #154018)

I've tweaked the test a little bit: added construct_called = true; in 2 places, used std::is_class<T>::value. That shouldn't change your intention. Tried and it works in C++17 but not in C++03.

Given that it didn't work earlier, not planning to fix it now.

vsapsai updated this revision to Diff 154326.Jul 5 2018, 3:03 PM
  • Use a better way to detect presence of construct with required signature. Clean up tests.

Don't know how other compilers will handle this but Clang accepts this
C++11-looking-but-accepted-in-C++03 code.

Quuxplusone added inline comments.Jul 5 2018, 5:29 PM
libcxx/include/memory
1470 ↗(On Diff #154326)

I think you should replace this ))) with )), void()) for absolute correctness (e.g. construct might plausibly return a pointer to the constructed object, and I think C++03 is okay with that).
Otherwise, awesome, this looks like what I'd expect. :)

vsapsai added inline comments.Jul 6 2018, 2:45 PM
libcxx/include/memory
1470 ↗(On Diff #154326)

The standard says the result of construct is not used but I haven't found any mention it must be void.

That being said, the proposed change fails for the C++03 macro-based decltype shim:

error: too many arguments provided to function-like macro invocation
                                    _VSTD::declval<_Args>()), void()),
                                                              ^

One option to fix that is to use

is_same
<
    decltype(_as_before_),
    decltype(_the_same_copy_pasted_argument_)
>

But I don't think this use case is worth such hacks.

Quuxplusone added inline comments.Jul 6 2018, 3:28 PM
libcxx/include/memory
1470 ↗(On Diff #154326)

I think the workaround is easier. Try

template <class _Alloc, class _Pointer, class _A0>
struct __has_construct<_Alloc, _Pointer, _A0, typename enable_if<
  is_same<
    decltype((_VSTD::declval<_Alloc>().construct(
      _VSTD::declval<_Pointer>(),
      _VSTD::declval<_A0>()
    ), void() )),
    void
  >::value
>::type> : std::true_type {};

i.e. with an extra set of parens to group the argument to the decltype macro: decltype(()) instead of decltype(). I also drive-by edited Args to A0, matching how it's done elsewhere in pseudo-variadic-templates in this file.

vsapsai updated this revision to Diff 154678.Jul 9 2018, 12:40 PM
  • Allow allocator construct to return a value, not just have return type void.
vsapsai added inline comments.Jul 9 2018, 12:49 PM
libcxx/include/memory
1470 ↗(On Diff #154326)

Thanks for the suggestion, Arthur, that worked.

Regarding _Args vs _A0, I am deliberately using _Args because it's not a manual pseudo-variadic-template expansion and it's not inside _LIBCPP_HAS_NO_VARIADICS block. But I don't feel strong about it, can change to _A0.

Why are we doing this?

I can't find the language in the C++03 specification that requires us to call an allocators construct method if it's present.

Why are we doing this?

I can't find the language in the C++03 specification that requires us to call an allocators construct method if it's present.

The main reason I've ended up doing this because I thought correct __has_construct is required for D48342. It's not the case but I think this change is useful on its own.

I agree that C++03 specification doesn't require to call construct method. Requirement for containers 23.1[lib.container.requirements]p9 mentions that allocator should be used for memory allocation:

Copy constructors for all container types defined in this clause copy an allocator argument from their respective first parameters. All other constructors for these container types take an Allocator& argument (20.1.6), an allocator whose value type is the same as the container’s value type. A copy of this argument is used for any memory allocation performed, by these constructors and by all member functions, during the lifetime of each container object. In all container types defined in this clause, the member get_allocator() returns a copy of the Allocator object used to construct the container.

And requirement for allocate 20.1.6[lib.allocator.requirements]table34 is not to construct objects:

Memory is allocated for n objects of type T but objects are not constructed.[...]

Also for specific containers you can have a requirement to construct a container using the specified allocator, without explaining what exactly does it mean. E.g. 23.2.1.1[lib.deque.cons]p3:

explicit deque(size_type n, const T& value = T(),
               const Allocator & = Allocator ());

Effects: Constructs a deque with n copies of value , using the specified allocator.

So these requirements tell that at some point you need to construct an object and you have to use an allocator but they don't explicitly spell out that you have to call construct.

My main motivation for this change is that in C++11 and later we do call construct if it is available and it is required per 23.2.1[container.requirements.general]p3:

For the components affected by this subclause that declare an allocator_type, objects stored in these components shall be constructed using the allocator_traits<allocator_type>::construct function and destroyed using the allocator_traits<allocator_type>::destroy function (20.7.8.2). These functions are called only for the container’s element type, not for internal types used by the container. [Note: This means, for example, that a node-based container might need to construct nodes containing aligned buffers and call construct to place the element into the buffer. — end note ]

I believe it is valuable and desirable to have the same behavior in different versions of C++ unless the semantic was explicitly changed. As far as I know, allocator_traits in C++11 wasn't supposed to be a breaking change.

Why are we doing this?

I can't find the language in the C++03 specification that requires us to call an allocators construct method if it's present.

I think it's being proposed under "quality of implementation."

It has only just now occurred to me: a quality implementation should probably never mismatch calls to construct and destroy. Does libc++ currently call destroy in C++03? Does it call destroy in C++03 after this patch? If this patch is making libc++ call construct but not destroy, that's a bug. If before this patch libc++ would call destroy but not construct, this patch is a bugfix! (And please add a test case for matched pairs of construct and destroy.)

libcxx/include/memory
1470 ↗(On Diff #154326)

Please change it (as far as I'm concerned, anyway). "_Args" plural implies a pack, and there's no pack here.

Why are we doing this?

I can't find the language in the C++03 specification that requires us to call an allocators construct method if it's present.

I think it's being proposed under "quality of implementation."

It has only just now occurred to me: a quality implementation should probably never mismatch calls to construct and destroy. Does libc++ currently call destroy in C++03? Does it call destroy in C++03 after this patch? If this patch is making libc++ call construct but not destroy, that's a bug. If before this patch libc++ would call destroy but not construct, this patch is a bugfix! (And please add a test case for matched pairs of construct and destroy.)

In C++03 destroy isn't called neither before my patch nor after. Will fix that. Though it's harder to find specification for destroy in C++03.

vsapsai updated this revision to Diff 154912.Jul 10 2018, 6:02 PM
  • In C++03 call allocator's destroy when available.
  • Rename _Args as it's not a variadic template pack.
ldionne requested changes to this revision.Dec 7 2018, 8:01 AM
  1. Just to make sure I understand; this patch has nothing to do with https://reviews.llvm.org/D48342, they are entirely orthogonal. Is this correct?
  2. Also, before this patch, the allocator's construct and destroy were NEVER called in C++03, but were called when available in C++11. After this patch, they are called when available in C++03 and in C++11. Is this correct?

If (2) is true, then I believe this patch is a worthwhile improvement in terms of quality-of-implementation, even though it's not mandated by the spec. People do have custom allocators, and this behavior change between C++03 and C++11 is quite subtle and surprising.

libcxx/include/memory
1466 ↗(On Diff #154912)

You can just say

template <class _Alloc, class _Pointer, class _Tp>
struct __has_construct<_Alloc, _Pointer, _Tp, typename __void_t<
    decltype(_VSTD::declval<_Alloc>().construct(_VSTD::declval<_Pointer>(), _VSTD::declval<_Tp>()))
>::type> : std::true_type { };
1482 ↗(On Diff #154912)

Here,

template <class _Alloc, class _Pointer>
struct __has_destroy<_Alloc, _Pointer, typename __void_t<
    decltype(_VSTD::declval<_Alloc>().destroy(_VSTD::declval<_Pointer>()))
>::type> : std::true_type { };
1726 ↗(On Diff #154912)

Here,

template <class _Tp, class _A0, class = typename enable_if<__has_construct<allocator_type, _Tp*, _A0 const&>::value>::type>
static void __construct(...);

and then below

template <class _Tp, class _A0, class = typename enable_if<!__has_construct<allocator_type, _Tp*, _A0 const&>::value>::type>
static void __construct(...);

This way you don't have to call __has_construct at the point of call and __construct is slightly more reusable.

libcxx/test/std/containers/sequences/vector/vector.cons/construct_iter_iter.pass.cpp
122 ↗(On Diff #154912)

These tests are specific to libc++ because C++03 does not mandate this behavior. Please either move them into test/libcxx or guard them with _LIBCPP_VERSION. I think the former is what we normally do in these cases and should be preferred.

libcxx/test/std/containers/sequences/vector/vector.cons/construct_iter_iter_alloc.pass.cpp
135 ↗(On Diff #154912)

Same as above, this is libc++ specific.

This revision now requires changes to proceed.Dec 7 2018, 8:01 AM
vsapsai updated this revision to Diff 177803.Dec 11 2018, 5:09 PM
  • Update __has_construct, __has_destroy according to review comments.
  • Tighten tests: custom allocators aren't mandated by C++03 but libc++ has the support.
vsapsai marked 3 inline comments as done.Dec 11 2018, 5:19 PM

Regarding the tests. I've moved most of new tests for custom allocators to test/libcxx/*. And in destroy.pass.cpp I'm just checking _LIBCPP_VERSION to avoid copying the test to a different file.

libcxx/include/memory
1466 ↗(On Diff #154912)

Done. One of the reasons to use is_same was to mimic __has_construct for C++11 and later. But I don't think there is really value in keeping that.

1726 ↗(On Diff #154912)

I've tried that (also updated how static void construct calls __construct) and it works with C++2a but fails with C++03. The error is

llvm-project/libcxx/include/memory:1725:21: error: class member cannot be redeclared
        static void __construct(allocator_type&, _Tp* __p, const _A0& __a0)
                    ^
llvm-project/libcxx/include/memory:1720:21: note: previous definition is here
        static void __construct(allocator_type& __a, _Tp* __p, const _A0& __a0)
                    ^

I haven't investigated further yet.

libcxx/include/memory
1726 ↗(On Diff #154912)

FWIW, I don't have a strong opinion here. I think Volodymyr's way instantiates slightly fewer intermediate types. It's also consistent with the definition of __destroy right below it. I don't know who we expect to be "reusing" allocator_traits<A>::__construct.

OTOH, in this case I don't see any concrete benefit to making the dispatch explicit at the callsite. (Whereas in D49317 (Ctrl+F "Got it") I needed the dispatch to be explicit at the callsite.) So refactoring __construct would be reasonable, although perhaps slower, and open a can of worms as to whether __destroy should also be refactored.

vsapsai marked an inline comment as done.Dec 11 2018, 5:31 PM
  1. Also, before this patch, the allocator's construct and destroy were NEVER called in C++03, but were called when available in C++11. After this patch, they are called when available in C++03 and in C++11. Is this correct?

If (2) is true, then I believe this patch is a worthwhile improvement in terms of quality-of-implementation, even though it's not mandated by the spec. People do have custom allocators, and this behavior change between C++03 and C++11 is quite subtle and surprising.

The described situation is correct. And I agree it can be surprising and tricky.

  1. Just to make sure I understand; this patch has nothing to do with https://reviews.llvm.org/D48342, they are entirely orthogonal. Is this correct?

They are not entirely orthogonal in sense that https://reviews.llvm.org/D48342 doesn't account for difference in allocator behaviour between C++03 and C++11. So it is better to have uniform construct behaviour before using it in implementing other fixes.

ldionne accepted this revision.Dec 19 2018, 11:28 AM
This revision is now accepted and ready to land.Dec 19 2018, 11:28 AM
This revision was automatically updated to reflect the committed changes.