Review implementation #46

wusatosi · 2024-12-06T03:36:40Z

DO NOT MERGE

This is intend to review the reference implementation.

Please use GitHub's PR review feature to review the reference implementation.

I am busy with finals, I will jump in when I have time.

Link to paper: https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2024/p0843r14.html

Currently in working as section 23. https://eel.is/c++draft/inplace.vector

Would recommend starting at: chapter

Goals:

Check if function signatures are consistent with Paper
Check if function attributes are appropriate
Check if implementation is valid (e.g. logical error)
Check if struct properties are consistent with Paper
Propose tests (you are allowed to add tests to review branch, I would not recommend updating the implementation itself).

Non-goals:

Large scale refactor
Code style recommendation
Adopting P3074 (unless comments made to how to leave room for P3074)

NOTE

All edits are on an orphan branch, so I would suggest we minimize the edits to the implementation.

cc @jbab @DeveloperPaul123

wusatosi · 2024-12-06T03:54:15Z

wusatosi · 2024-12-06T03:58:00Z

Known faults:

overview section 6, all trivial copy/move construable requirement.
overview section 6, all trivial copy/move assignable requirement.

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

wusatosi · 2024-12-11T05:28:22Z

tests/beman/inplace_vector/spec.test.cpp

+      // TODO: This type is also trivially destructible
+      // ~T() {}


Question: Is it possible to construct a trivially copy constructable struct that is not trivially destructable ?

Answer: no,

is_trivially_copy_constructible is the same as is_trivially_constructible<T, const T&>, which in turn is requires is_constructible<T, const T&> where all operations are known to be trivial. is_constructible is defined in https://eel.is/c++draft/type.traits#meta.unary.prop-9, which refers to a variable, so the destructor is included.

Suggested change

// TODO: This type is also trivially destructible

// ~T() {}

// trivially copy constructible implies trivially destructible

tests/beman/inplace_vector/spec.test.cpp

jbab · 2024-12-16T16:59:51Z

tests/beman/inplace_vector/spec.test.cpp

+  ASSERT_EQ(Counting::num_objects, v.size() + t.size());
+  t = rv();
+  EXPECT_TRUE((std::is_same_v<decltype(t = rv()), X &>));
+  EXPECT_EQ(Counting::num_objects, 2 * v.size()); // t[4] has been destroyed


Test fails because inplace_vector's destructor is not properly implemented.

jbab · 2024-12-16T17:01:58Z

tests/beman/inplace_vector/spec.test.cpp

+  ASSERT_EQ(Counting::num_objects, 5);
+  a.~X();
+  EXPECT_TRUE(std::is_void_v<decltype(a.~X())>);
+  EXPECT_EQ(Counting::num_objects, 0);


Test fails because inplace_vector's destructor is not properly implemented.

wusatosi · 2024-12-16T18:35:55Z

Desturctor chain for faulty test (inplace_vector<Counter, Capacity>)

// sturct inplace_vector

  //   constexpr ~inplace_vector();
  // from base-class, trivial if is_trivially_destructible_v<__T> &&
  // is_trivially_copy_assignable_v<__T>

Base class:

// sturct __non_trivial

  using __data_t =
      conditional_t<!is_const_v<__T>, __aligned_storage2<__T, __N>,
                    const __aligned_storage2<remove_const_t<__T>, __N>>;
  __data_t __data_{}; // BUGBUG: test SIMD types
  __size_type __size_ = 0;
// ...
  constexpr ~__non_trivial() = default;

Data Member:

template <class __T, size_t __N> struct __aligned_storage2 {
  alignas(__T) byte __d[sizeof(__T) * __N];
  constexpr __T *__data(size_t __i) noexcept {
    __IV_EXPECT(__i < __N);
    return reinterpret_cast<__T *>(__d) + __i;
  }
  constexpr const __T *__data(size_t __i) const noexcept {
    __IV_EXPECT(__i < __N);
    return reinterpret_cast<const __T *>(__d) + __i;
  }
};

destructor is not called for elements in type erased storage.

Edit: I think adding this destruct overload to __non_trivial should be sufficient:

// struct __non_trivial
  constexpr ~__non_trivial() requires(is_trivially_destructible_v<__T>) = default;
  constexpr ~__non_trivial() {
      destroy(__data(), __data() + __size();
  }

No need to add this to the other base class due to their trivial nature.

CC @jbab

wusatosi · 2024-12-16T20:11:08Z

tests/beman/inplace_vector/spec.test.cpp

+// 3 For any N, inplace_vector<T, N>::iterator and inplace_vector<T,
+// N>::const_iterator meet the constexpr iterator requirements.
+//
+// TODO: Test this.


There are variations of constexpr requirement, and this depends on to be deprecated trivial type trait. We should split this out into individual files.

Specifically:

<= C++26 constexpr when is_trivial

>C++26 constexpr for wider T using trivial union, omit any reference to is_trivial

The mentioned requirement refers to the iterators of inplace_vector, not inplace_vector itself. The constexpr requirement for the iterators is independent from T and its triviality, there is no change planned in >C++26, afaik.

Ah I commented on the wrong lines, should be:

inplace_vector/tests/beman/inplace_vector/spec.test.cpp

Lines 518 to 521 in 06855a2

// 4 For any N > 0, if is_trivial_v<T> is false, then no inplace_vector<T, N>

// member functions are usable in constant expressions.

//

// This would be tested in subsequent method tests.

wusatosi · 2024-12-17T00:07:06Z

include/beman/inplace_vector/inplace_vector.hpp

+  using __data_t =
+      conditional_t<!is_const_v<__T>, __aligned_storage2<__T, __N>,
+                    const __aligned_storage2<remove_const_t<__T>, __N>>;
+  __data_t __data_{}; // BUGBUG: test SIMD types
+  __size_type __size_ = 0;


we could use a union based lifetime delegation here instead of byte array based type erasure for supporting trivial union based constexpr:

union U { U() { } ~U() { } __T storage[__N]; }; U __data_; __size_type __size_ = 0;

This should cover both non-trivial base case and trivial union based constexpr implementation. I think this also omits the need to use aligned storage as well?

Edit: We will need to try this out, and also be aware of code gen issues mentioned in the paper.

I think there is no point switch to a union based approach before the issues addressed in P3074are fixed. As of now, we would need to add a constructor and destructor to the union (as you write above), therefore the storage (and thus the whole inplace_vector) could no longer be trivially constructible and trivially destructible.
Note that the requirement

If is_trivially_destructible_v is true, then:
IV has a trivial destructor

holds for any T, independent of its triviality (and thus, as of now, storage type).

A general notes on the tests:

Tests should achieve a high (ideally full) coverage of all the branches, this includes not only runtime branches, but also "implementation branches" due to, e.g., different storage types for trivial vs non-trivial types.

Ideally, we make the tests independent from the implementation (black box) and only derive the test cases from the specification.

The current specification tells us that there must be at least two implementation branches for trivial and non-trivial T, so we should at least test with such types

The requirements on special member functions provide more information, for example

If is_trivially_destructible_v is true, then:
IV has a trivial destructor

this means that we need to test (at least) with 1. a trivial type, 2. a non-trivial type which is trivially destructible, 3. a non-trivial type which is not trivially destructible.
Note that this does not only affect the test of the destructor, we don't know which other functions may be affected by these implementation branches if we treat the code under test as a black box.

Further requirements on other other special member functions make it even more complicated

In addition there are some special requirements (and thus an implementation branch) for size 0.

All in all, we need to determine the proper combination of types and then run the full test suite on them.
Once we have that we can fix the implementation and experiment with improvements.
(We might even be able to port back the implementation to C++20, ...)

I have some WIP with this respect which I hope I can commit shortly.

we would need to add a constructor and destructor to the union (as you write above), therefore the storage (and thus the whole inplace_vector) could no longer be trivially constructible and trivially destructible.

You are right, I missed that.

I support backporting to C++ 20, the original aim was C++20 for trivial constexpr, C++17 for basic functionality.

We might need to also test for const T.

…for [container.rev.reqmts]

tests/beman/inplace_vector/CMakeLists.txt

tests/beman/inplace_vector/spec.test2.experimental.cpp

wusatosi · 2024-12-17T18:44:09Z

tests/beman/inplace_vector/spec.test2.experimental.cpp

+// We know from the specification of inplace_vector that there are separate
+// implementation branches for trivial and non-trivial types (due to constexpr
+// support). Furthermore, we have the requirements that the triviality of
+// inplace_vector's SMFs depends on the triviality of the element type T:


I hope this is not a stupid question.
What is SMF?

Thanks for asking. I'm using this all the time so I didn't even notice it's not obvious.
SMF = special member function, i.e. default constructor, copy constructor, move constructor, destructor, copy assignment, move assignment.

Thx for the clarification 👌

JeffGarland · 2024-12-20T02:35:22Z

include/beman/inplace_vector/inplace_vector.hpp

+*/
+
+// TODO(River): using namespace std below, we need to specify
+// beman::from_range_t


so the actual check if gcc has this is gcc14 and in c++23 mode. Or perhaps better, the feature macro #ifdef __cpp_lib_containers_ranges see also https://en.cppreference.com/w/cpp/ranges/from_range

Given we have intention to backport this to C++20 and earlier, would it make sense to use a custom beman::from_range here?

JeffGarland · 2024-12-20T02:38:15Z

include/beman/inplace_vector/inplace_vector.hpp

+static constexpr void __assert_failure(char const *__file, int __line,
+                                       char const *__msg) {
+  if consteval {
+    throw __msg; // TODO: std lib implementer, do better here


well and even more is that this will be legal in c++26 after Poland https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2024/p3068r6.html was voted in

I'd suggest we probably don't need this in c++26 where assert and static assert are upgraded to allow user error messages. Ironically I think that's used directly in some static asserts below. Also, a reminder is source_location is probably a better way to get that than the macro approach

👌 thanks for the input, I am not a huge fan of this static asset infrastructure as well.

I will likely remove them or refactor them with static assert if possible

wusatosi · 2025-01-14T21:27:33Z

ohno!

wusatosi · 2025-01-14T21:28:03Z

oh no I can't change it back :(

wusatosi added 2 commits December 5, 2024 22:29

base

c15cb26

to review

b8413d6

wusatosi and others added 3 commits December 10, 2024 22:50

add basic gtest framework

082a389

add outline for spec test

feaed7d

Update CMakeLists.txt

07b2a79

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

wusatosi commented Dec 11, 2024

View reviewed changes

jbab reviewed Dec 16, 2024

View reviewed changes

tests/beman/inplace_vector/spec.test.cpp Show resolved Hide resolved

Added tests for [container.reqmts]

6b9763f

jbab reviewed Dec 16, 2024

View reviewed changes

wusatosi commented Dec 16, 2024

View reviewed changes

wusatosi commented Dec 17, 2024

View reviewed changes

Added second spec test suite (experimental, suggestion), added tests …

06855a2

…for [container.rev.reqmts]

github-actions bot reviewed Dec 17, 2024

View reviewed changes

wusatosi commented Dec 17, 2024

View reviewed changes

JeffGarland reviewed Dec 20, 2024

View reviewed changes

wusatosi added 10 commits December 24, 2024 22:53

refactor triviality tests

1678966

update trivial type wording

e7ec325

format

1897df8

expand test structure

dc35d0b

add erase checks

8ad8d91

complete more erasure

b9eab69

complete modifiers/erasesingle modifier/popback

a89ca10

Add minimal for reserve shrink

04674e1

Fix PopBack test

c59891b

Include Reserve and shrink

a9355cd

wusatosi added 3 commits January 14, 2025 14:41

lint

27405bd

Implement erase tests

7c73723

add pushback/ emplaceback tests

8ef1383

wusatosi changed the base branch from review-dummy to main January 14, 2025 21:26

wusatosi closed this Jan 14, 2025

wusatosi mentioned this pull request Jan 15, 2025

Ensure implementation is up to spec #55

Merged

wusatosi deleted the review branch January 30, 2025 22:43

	// TODO: This type is also trivially destructible
	// ~T() {}
	// trivially copy constructible implies trivially destructible

	// 4 For any N > 0, if is_trivial_v<T> is false, then no inplace_vector<T, N>
	// member functions are usable in constant expressions.
	//
	// This would be tested in subsequent method tests.

Review implementation #46

Review implementation #46

Uh oh!

Conversation

wusatosi commented Dec 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

DO NOT MERGE

Goals:

Non-goals:

NOTE

Uh oh!

wusatosi commented Dec 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wusatosi commented Dec 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wusatosi commented Dec 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wusatosi Dec 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wusatosi Dec 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jbab Dec 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JeffGarland Dec 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wusatosi Dec 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wusatosi commented Jan 14, 2025

Uh oh!

wusatosi commented Jan 14, 2025

Uh oh!

Reviewers

Assignees

wusatosi commented Dec 6, 2024 •

edited

Loading

wusatosi commented Dec 6, 2024 •

edited

Loading

wusatosi commented Dec 6, 2024 •

edited

Loading

wusatosi commented Dec 16, 2024 •

edited

Loading

wusatosi Dec 16, 2024 •

edited

Loading

wusatosi Dec 17, 2024 •

edited

Loading

jbab Dec 17, 2024 •

edited

Loading

JeffGarland Dec 20, 2024 •

edited

Loading

wusatosi Dec 21, 2024 •

edited

Loading