Ensure implementation is up to spec #55

wusatosi · 2025-01-15T20:51:08Z

The goal of this PR is to add basic spec tests so the implementation would be minimally useable.

It would be fine to update the implementation itself in this branch.

See spec: https://eel.is/c++draft/inplace.vector

Meant to replace #46 which I accidentally broke.

cc: @jbab

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

…for [container.rev.reqmts]

include/beman/inplace_vector/inplace_vector.hpp

tests/beman/inplace_vector/spec.test.cpp

wusatosi · 2025-01-19T04:28:40Z

The only thing left should be [sequence.reqmts]

jbab

Some remarks on constructor.test.cpp. I still need to go through the other files.

jbab · 2025-01-19T16:32:42Z

tests/beman/inplace_vector/constructors.test.cpp

+  if (std::is_same_v<T, NonTriviallyDefaultConstructible> ||
+      std::is_same_v<T, NonTrivial>) {
+
+    IV mid_correct;
+    for (auto i = 0ul; i < mid_size; ++i)
+      mid_correct.emplace_back();
+
+    EXPECT_EQ(mid, mid_correct);
+  }


When I first read this I had to think a bit what this is for. It is for the the effect "... with n default-inserted elements". This means that the values are value-initialized (see https://en.cppreference.com/w/cpp/named_req/DefaultInsertable). Therefore there is no need to restrict the test to non trivially default constructible types. It should work with all default constructible types (all of the test types are). The test is that each element is equal to T{}.

Suggested change

if (std::is_same_v<T, NonTriviallyDefaultConstructible> ||

std::is_same_v<T, NonTrivial>) {

IV mid_correct;

for (auto i = 0ul; i < mid_size; ++i)

mid_correct.emplace_back();

EXPECT_EQ(mid, mid_correct);

}

// All elements of mid are T{}

EXPECT_EQ(std::count(mid.begin(), mid.end(), T{}), mid.size());

We have this project now set to c++23 -- I'd suggest we simplify this code using ranges algo

EXPECT_EQ(std::ranges::count(mid, T{}), mid.size());

Alternatively std::ranges::all_of would work as well. Comment applies below as well

The problem here is that types other than this two does not have defined value:
e.g.:

// A trivial type struct Trivial { int value; friend constexpr bool operator==(Trivial x, Trivial y) = default; };

The value is not defined for implicit default constructor. Equality check here is invalid and undefined.

Only NonTriviallyDefaultConstructible and NonTrivial have their member variables defined.

// A type which is not trivially default constructible (and thus not trivial), // and all other SMFs are trivial. struct NonTriviallyDefaultConstructible { int value = 0; friend constexpr bool operator==(NonTriviallyDefaultConstructible x, NonTriviallyDefaultConstructible y) = default; };

The problem here is that types other than this two does not have defined value: e.g.:

// A trivial type struct Trivial { int value; friend constexpr bool operator==(Trivial x, Trivial y) = default; };

The value is not defined for implicit default constructor. Equality check here is invalid and undefined.

Make it defined is easy and this should be trivial?

struct Trivial { int value = 0; bool operator==( const Trivial& ) const = default; };

https://godbolt.org/z/63nc3hqGr

Sure, so basically there's no testing this because anything trivial can't do any initialization and so any comparison will be UB -- no matter the technique used to compare.

@JeffGarland, @wusatosi you are right, I forgot about my own implementation of the test types, of course we need a condition here.
What bothers me a bit about the current condition

if (std::is_same_v<T, NonTriviallyDefaultConstructible> || std::is_same_v<T, NonTrivial>)

is that it mentions the test types directly. We may add to them or remove some of them in the future or rename them, so this becomes brittle.
So I'm suggesting to focus on the type properties which are relevant here. This is obviously

!std::is_trivially_default_constructible_v<T>

But there are also trivially default constructible types which can be value-initialised to a defined value (all zeros). These are the scalar types. So or condition becomes

if (std::is_scalar_v<T> || !std::is_trivially_default_constructible_v<T>) ...

Agree?

This makes much more sense, thank you so much for your input. I will apply your suggestion once I have time.

In addition, if T is an aggregate type, the expression T{} performs aggregate initialization, which means that the member value will be initialized to zero. There are a few types in the test suite that qualify for this.

tests/beman/inplace_vector/constructors.test.cpp

jbab · 2025-01-19T17:27:28Z

tests/beman/inplace_vector/constructors.test.cpp

+TYPED_TEST(Constructors, CopyIter) {
+  // template<class InputIterator>
+  //   constexpr inplace_vector(InputIterator first, InputIterator last);
+  // Effects: Constructs an inplace_vector equal to the range [first, last).
+  // Complexity: Linear in distance(first, last).
+
+  using IV = TestFixture::IV;
+
+  auto reference = this->unique();
+  IV device(reference.begin(), reference.end());
+
+  EXPECT_EQ(device, reference);
+}


What this fails to test is whether the constructor works with an iterator which is InputIterator and nothing else. I.e. single pass, going forward only.

Ah I see, is there any standard library facility to warp an existing iterator to make a single pass forward only iterator?

Not that I'm aware of.

Alright noted.

JeffGarland · 2025-01-19T23:29:21Z

tests/beman/inplace_vector/constructors.test.cpp

+  }
+
+  {
+    auto mid = std::midpoint(0ul, reference.size());


0ul is preferably 0uz in c++23 to ensure it's size_t

thanks for the input!

I ended up reverting this, we would like to eventually aim to support C++20.

ok, but the assumption that 0ul == size_t may not hold everywhere. I realize it's verbose but I think std::size_t(0) would be more portable.

You're right here

This reverts commit 451f0c5. This is because `uz` is a C++23 feature, we will eventually support earlier versions

wusatosi · 2025-01-20T18:17:31Z

Turns out C++20 support is doable with minimal changes, I will update the docs in another PR but set the checks here for C++20.

wusatosi and others added 17 commits January 15, 2025 15:42

add basic gtest framework

cde9f44

add outline for spec test

5eb8151

Update CMakeLists.txt

0d29b77

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

Added tests for [container.reqmts]

d953f95

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

d33b8e8

…for [container.rev.reqmts]

update trivial type wording

97345e6

format

6b35bbc

expand test structure

c854038

add erase checks

73dee7b

complete more erasure

caa14ad

complete modifiers/erasesingle modifier/popback

b8d30f8

Add minimal for reserve shrink

f73c4fe

Fix PopBack test

3b74798

Include Reserve and shrink

e970fa2

lint

b883947

Implement erase tests

e75d44b

add pushback/ emplaceback tests

9ca550e

wusatosi requested review from DeveloperPaul123, Hels15 and camio as code owners January 15, 2025 20:51

wusatosi added 6 commits January 15, 2025 16:04

Fix emplace back

2a8b99e

fold spec.test2 into spec.test

70cad6c

refactor triviality tests

0d146a7

Fix destructor

ef169f9

Fix Triviality/ZeroSized

81903c2

lint

ddb19ac

wusatosi marked this pull request as draft January 15, 2025 22:25

fix triviality

c1bdbac

jbab reviewed Jan 16, 2025

View reviewed changes

include/beman/inplace_vector/inplace_vector.hpp Outdated Show resolved Hide resolved

Fix Tests

f238186

wusatosi added 2 commits January 16, 2025 11:08

fix werror

1c1eb86

remove false positive werror check

ab82c73

wusatosi commented Jan 16, 2025

View reviewed changes

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

wusatosi added 13 commits January 17, 2025 12:46

add tests for insert

cbfe0c4

implement insert ranges series

0d37485

add data tests

bb97f12

Add testcase for size and resize

2bec84a

minor addition for resize test

f7a3b74

fix test

955e9f2

dump [sequence.reqmts]

d135f4a

remove #if 0 chuck

30cb4a0

Fix compile

634b185

Added SizedDefault Test

c300258

Split test file

d446946

Format Fix

2c9c721

Finish constructor test suite

e9d8635

wusatosi marked this pull request as ready for review January 19, 2025 04:26

jbab reviewed Jan 19, 2025

View reviewed changes

Finish [sequence.reqmts]

fea9b08

JeffGarland reviewed Jan 19, 2025

View reviewed changes

wusatosi mentioned this pull request Jan 20, 2025

specify namespace std #57

Merged

wusatosi added 4 commits January 20, 2025 12:14

use uz

451f0c5

adopt suggestion

e0ff597

beman::bad_alloc => std::bad_alloc

5b23f5a

Revert "use uz"

212c829

This reverts commit 451f0c5. This is because `uz` is a C++23 feature, we will eventually support earlier versions

wusatosi and others added 2 commits January 20, 2025 13:19

allow c++20

d1f1705

Improvements to tests

fa0eacb

wusatosi merged commit 5340a03 into main Jan 24, 2025
32 checks passed

Ensure implementation is up to spec #55

Ensure implementation is up to spec #55

Uh oh!

Conversation

wusatosi commented Jan 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

wusatosi commented Jan 19, 2025

Uh oh!

jbab left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JeffGarland Jan 19, 2025 • 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 Jan 20, 2025 • 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!

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!

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!

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!

wusatosi commented Jan 20, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

wusatosi commented Jan 15, 2025 •

edited

Loading

JeffGarland Jan 19, 2025 •

edited

Loading

wusatosi Jan 20, 2025 •

edited

Loading