Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

make_shared() For Arrays #309

Merged
merged 62 commits into from
Apr 8, 2020

Conversation

AdamBucior
Copy link
Contributor

@AdamBucior AdamBucior commented Nov 18, 2019

Description

Resolves #33. It's quite basic implementation but it should work. I would be grateful for any tips on how to upgrade it.

Checklist

Be sure you've read README.md and understand the scope of this repo.

If you're unsure about a box, leave it unchecked. A maintainer will help you.

  • Identifiers in product code changes are properly _Ugly as per
    https://eel.is/c++draft/lex.name#3.1 or there are no product code changes.
  • The STL builds successfully and all tests have passed (must be manually
    verified by an STL maintainer before automated testing is enabled on GitHub,
    leave this unchecked for initial submission).
  • These changes introduce no known ABI breaks (adding members, renaming
    members, adding virtual functions, changing whether a type is an aggregate
    or trivially copyable, etc.).
  • These changes were written from scratch using only this repository,
    the C++ Working Draft (including any cited standards), other WG21 papers
    (excluding reference implementations outside of proposed standard wording),
    and LWG issues as reference material. If they were derived from a project
    that's already listed in NOTICE.txt, that's fine, but please mention it.
    If they were derived from any other project (including Boost and libc++,
    which are not yet listed in NOTICE.txt), you must mention it here,
    so we can determine whether the license is compatible and what else needs
    to be done.

@AdamBucior AdamBucior requested a review from a team as a code owner November 18, 2019 21:02
@SuperWig
Copy link
Contributor

Uses of max (or min) need to be surrounded by parens e.g.

STL/stl/inc/chrono

Lines 47 to 50 in 1980e1a

_NODISCARD static constexpr _Rep(max)() noexcept {
// get largest value
return (numeric_limits<_Rep>::max)();
}

stl/inc/memory Show resolved Hide resolved
@AdamBucior
Copy link
Contributor Author

Uses of max (or min) need to be surrounded by parens e.g.

STL/stl/inc/chrono

Lines 47 to 50 in 1980e1a

_NODISCARD static constexpr _Rep(max)() noexcept {
// get largest value
return (numeric_limits<_Rep>::max)();
}

Thanks for info! I didn't know that.

@AdamBucior
Copy link
Contributor Author

If I understand the log correctly clang-format requires me to change _STD rank_v<_Ty> > 0 to _STD rank_v<_Ty>> 1 which looks weird and is weird, because I used clang-format on whole file and everything should be correct. Is it a bug?

@BillyONeal
Copy link
Member

I think it might be clang-format 9.0.0 not understanding variable templates?

@cpplearner
Copy link
Contributor

If I understand the log correctly clang-format requires me to change _STD rank_v<_Ty> > 0 to _STD rank_v<_Ty>> 1 which looks weird and is weird, because I used clang-format on whole file and everything should be correct. Is it a bug?

Yeah it's a bug of clang-format 9.0.0. See https://bugs.llvm.org/show_bug.cgi?id=42404

@BillyONeal
Copy link
Member

So looks like the suggested workaround is to add extra parenthesis. So do something like:

// TRANSITION, LLVM-42404
if ((_STD rank_v<X>) > 0) {
    // as before
}

stl/inc/memory Outdated Show resolved Hide resolved
stl/inc/memory Outdated Show resolved Hide resolved
stl/inc/memory Outdated Show resolved Hide resolved
stl/inc/memory Outdated Show resolved Hide resolved
stl/inc/memory Outdated Show resolved Hide resolved
stl/inc/memory Outdated Show resolved Hide resolved
stl/inc/memory Show resolved Hide resolved
stl/inc/memory Outdated Show resolved Hide resolved
stl/inc/memory Outdated Show resolved Hide resolved
stl/inc/memory Show resolved Hide resolved
@StephanTLavavej
Copy link
Member

@SuperWig said:

Uses of max (or min) need to be surrounded by parens

Yep! The reason for this is that Windows.h defines function-like macros for min and max, and the user might have included Windows.h before any STL headers. Although users can suppress this by defining NOMINMAX, the STL can't assume that users have done that. Adding extra parentheses here prevents the preprocessor from expanding function-like macros (very useful although not widely known). It also prevents Argument-Dependent Lookup although we never use extra parentheses for that purpose.

Within the STL, we usually use _Min_value and _Max_value so we don't need to drag in the entire <algorithm> header:

STL/stl/inc/utility

Lines 26 to 38 in 1980e1a

// FUNCTION TEMPLATE _Min_value
template <class _Ty>
_Post_equal_to_(_Right < _Left ? _Right : _Left) constexpr const _Ty& _Min_value(
const _Ty& _Left, const _Ty& _Right) noexcept(noexcept(_Right < _Left)) {
return _Right < _Left ? _Right : _Left;
}
// FUNCTION TEMPLATE _Max_value
template <class _Ty>
_Post_equal_to_(_Left < _Right ? _Right : _Left) constexpr const _Ty& _Max_value(
const _Ty& _Left, const _Ty& _Right) noexcept(noexcept(_Left < _Right)) {
return _Left < _Right ? _Right : _Left;
}

However, we need the extra parens for numeric_limits and <random> functions named min and max.

stl/inc/memory Outdated Show resolved Hide resolved
stl/inc/memory Outdated Show resolved Hide resolved
stl/inc/memory Outdated Show resolved Hide resolved
stl/inc/memory Outdated Show resolved Hide resolved
stl/inc/memory Outdated Show resolved Hide resolved
@AdamBucior AdamBucior closed this Nov 29, 2019
@AdamBucior AdamBucior reopened this Nov 29, 2019
@AdamBucior
Copy link
Contributor Author

Sorry misclick

@StephanTLavavej
Copy link
Member

Changelog for the last batch of changes, which were in response to things that I found during my exhaustive review and didn't submit code review comments for:

Add noexcept to internal machinery.

Instead of testing (rank_v<_Ty>) > 0, simply test is_array_v<_Ty>.

Change _Reverse_destroy_multidimensional_n_guard to be an aggregate. (Note that Clang 10 doesn't support CTAD for aggregates.)

Rework the optimization metaprogramming in _Uninitialized_copy_multidimensional. First, _Ptr_copy_cat<_Ty*, _Ty*>::_Really_trivial was incorrect. This is because _Ptr_copy_cat checks is_trivially_assignable_v, which will be false when _Ty is an array. Instead, we should test is_trivial_v<_Ty>. (We don't need the rest of what _Ptr_copy_cat does, because we're not working with user-defined iterators, and our source and destination are the same type.) Second, is_trivial_v<_Ty> can be the first thing we test; when we're working with trivial elements, we can perform a single _Copy_memmove, regardless of whether these are single-dimensional or multi-dimensional arrays. Finally, for is_array_v<_Ty>, it was strange (although correct) to construct an RAII guard for each iteration of the loop. We can lift this out and simply update the guard's index. (Same transformation for the following algorithms.)

Similarly for _Uninitialized_value_construct_multidimensional_n, _Use_memset_value_construct_v<_Ty*> was incorrect as it checks is_scalar which is false for arrays. We need to use remove_all_extents_t<_Ty>, which I've been referring to as _Item. Also similarly, we can perform this _Zero_range optimization at the top level. Finally, we should consistently use _Idx instead of _Count to iterate (also changed below).

In _Uninitialized_fill_multidimensional_n, add the // intentionally copy, not fill comment (previously applied to the allocator version).

Mark _Get_ptr() as _NODISCARD and noexcept.

Change pre-existing code for consistency: upgrade _Ref_count_obj_alloc2 to _Ref_count_obj_alloc3. The difference is that we store _Rebind_alloc_t<_Alloc, _Ty> so we can directly use it later. (We still need to rebind for _Delete_this.) Also add a static_assert verifying that we're following the Standard's remove_cv_t wording.

Change _Reverse_destroy_multidimensional_n_al_guard and its associated algorithms similarly to the non-allocator versions.

In _Uninitialized_copy_multidimensional_al, change _Uses_default_construct<_Alloc, _Ty*, _Ty> to _Uses_default_construct<_Alloc, _Item*, const _Item&>. This is because the provided _Alloc is for the _Item (aka remove_all_extents_t<_Ty>) that we ultimately construct from a const lvalue.

In _Uninitialized_value_construct_multidimensional_n_al, similarly change the _Zero_range metaprogramming.

In _Uninitialized_fill_multidimensional_n_al, slightly adjust _Uses_default_construct<_Alloc, _Ty*, _Ty> to _Uses_default_construct<_Alloc, _Ty*, const _Ty&> because we construct from a const lvalue, and that distinction could be important.

Change _Ref_count_unbounded_array_alloc to store _Rebind_alloc_t<_Alloc, remove_all_extents_t<_Ty>> so we don't need to rebind it later. Also static_assert remove_cv_t.

Adjust the metaprogramming in _Destroy(). is_trivially_destructible<_Element_type> was correct (_Element_type removes the unbounded array, and the type trait "recurses" to the non-array item), but is_trivially_destructible<_Item> is what we're ultimately interested in, and will be more consistent with other control blocks.

Also, _Rebound is never a reference now, and we have the _Item typedef.

Fix _Ref_count_bounded_array_alloc comment; "for object" should be "for bounded array". Similarly change its implementation. Here, is_trivially_destructible<_Ty> was correct (_Ty is a bounded array), butis_trivially_destructible<_Item> lets us be consistent with the unbounded array control block.

stl/inc/memory Show resolved Hide resolved
stl/inc/memory Show resolved Hide resolved
stl/inc/memory Show resolved Hide resolved
stl/inc/memory Outdated Show resolved Hide resolved
* Include <cstdlib> for malloc.
* Include <new> for bad_alloc.
* Include <string_view>, newly used.
* The precise Word Of Power is that `operator new` is "replaced".
* Use size_t for vector indices; this respects 64-bit (not that these
vectors can be large). Additionally, we conventionally never say
`unsigned` - we would say `unsigned int`.
* Remove artificial scopes around try/catch - they already introduce
scopes, so this was just extra indentation.
* Instead of constructing a std::string in a catch block, compare
against a string_view literal, avoiding dynamic memory allocation.
* Conventionally use preincrement.
* Initialize InitialValue::value instead of reassigning it.

* Test `canCreate > 0` instead of directly testing an integer for being
nonzero.

* assert_shared_use_get() is an observer.

* Always use template argument deduction when calling
assert_shared_use_get().

* Add many missing calls to assert_shared_use_get().

* Test plain int (and arrays of known/unknown bound) as it's sensitive
to the difference between default-init and value-init. The other types
tested have constructors.

* Iterating through vectors with `int&` was correct, but `const auto&`
is more conventional in our tests for pure observation. (This is more
important for maps, where getting the element type wrong is a real
possibility.)
@StephanTLavavej
Copy link
Member

@Weheineman Changelog for my test changes - they were mechanical changes so I just went and fixed them instead of leaving code review comments.

  • Brace all control flow.
  • Include <cstdlib> for malloc.
  • Include <new> for bad_alloc.
  • Include <string_view>, newly used.
  • The precise Word Of Power is that operator new is "replaced".
  • Use size_t for vector indices; this respects 64-bit (not that these
    vectors can be large). Additionally, we conventionally never say
    unsigned - we would say unsigned int.
  • Remove artificial scopes around try/catch - they already introduce
    scopes, so this was just extra indentation.
  • Instead of constructing a std::string in a catch block, compare
    against a string_view literal, avoiding dynamic memory allocation.
  • Conventionally use preincrement.
  • Initialize InitialValue::value instead of reassigning it.
  • Test canCreate > 0 instead of directly testing an integer for being
    nonzero.
  • assert_shared_use_get() is an observer.
  • Always use template argument deduction when calling
    assert_shared_use_get().
  • Add many missing calls to assert_shared_use_get().
  • Test plain int (and arrays of known/unknown bound) as it's sensitive
    to the difference between default-init and value-init. The other types
    tested have constructors.
  • Iterating through vectors with int& was correct, but const auto&
    is more conventional in our tests for pure observation. (This is more
    important for maps, where getting the element type wrong is a real
    possibility.)

Copy link
Member

@StephanTLavavej StephanTLavavej left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe that this is ready for checkin! 😸

stl/inc/memory Outdated Show resolved Hide resolved
stl/inc/memory Outdated Show resolved Hide resolved
stl/inc/memory Outdated Show resolved Hide resolved
stl/inc/memory Outdated Show resolved Hide resolved
stl/inc/memory Outdated Show resolved Hide resolved
tests/std/tests/P0674R1_make_shared_for_arrays/test.cpp Outdated Show resolved Hide resolved
tests/std/tests/P0674R1_make_shared_for_arrays/test.cpp Outdated Show resolved Hide resolved
* Reorder friendship to match the Standard's declaration order.

* Extract `_Element_size` for simplicity.

* Support extended alignment in `_Allocate_flexible_array`.

* Add `_Deallocate_flexible_array` to handle extended alignment.

* Mark guard constructors as TRANSITION, P1771R1.

* Handle non-pointer iterators when calling `destroy_at()`.

* Add `static_assert` to array control blocks.

* Add `HighlyAligned` test coverage.

* Other test improvements.
stl/inc/memory Show resolved Hide resolved
stl/inc/memory Show resolved Hide resolved
stl/inc/memory Show resolved Hide resolved
stl/inc/memory Show resolved Hide resolved
stl/inc/memory Show resolved Hide resolved
@AdamBucior
Copy link
Contributor Author

Thanks for all your changes and review of this PR! I'm really glad that this PR finally got approved (and hopefully merges soon). Thanks again!

@StephanTLavavej StephanTLavavej merged commit a099e85 into microsoft:master Apr 8, 2020
@StephanTLavavej
Copy link
Member

Thanks for this C++20 feature, @AdamBucior! And thanks for the test coverage, @Weheineman! And the extended alignment support, @CaseyCarter! And the integer overflow resistance, @BillyONeal! make_shared<😸[5]>()!

@AdamBucior AdamBucior deleted the make_shared-for-arrays branch April 8, 2020 19:39
CaseyCarter added a commit to CaseyCarter/STL that referenced this pull request Feb 23, 2021
We introduced a couple of unguarded uses in `<memory>` in microsoft#309 "P0674R1 `make_shared` For Arrays".

Fixes DevCom-1346191.
CaseyCarter added a commit to CaseyCarter/STL that referenced this pull request Feb 23, 2021
We introduced a couple of unguarded uses in `<memory>` in microsoft#309 "P0674R1 `make_shared` For Arrays".

Fixes DevCom-1346191.
Fixes AB-1283107.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cxx20 C++20 feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

P0674R1 make_shared() For Arrays
8 participants