Skip to content

Fix for issue/141: cetl::get_if should should match only on a unbounded_variant #185

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.

Sign up for GitHub

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

Merged
merged 9 commits into from
Apr 15, 2025
Merged

Fix for issue/141: cetl::get_if should should match only on a unbounded_variant #185

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
88d0e20
Attempt to fix issue 141 #verification #docs #sonar
serges147 Apr 10, 2025
bb6bf12
Attempt to fix issue 141 #verification #docs #sonar
serges147 Apr 10, 2025
4d8cf5c
Attempt to fix issue 141 #verification #docs #sonar
serges147 Apr 10, 2025
3356227
Attempt to fix issue 141 #verification #docs #sonar
serges147 Apr 10, 2025
9703ef8
Merge branch 'main' into issue/141_ub_get
serges147 Apr 14, 2025
04910fb
Increase coverage for issue 141 #sonar
serges147 Apr 14, 2025
0b5d625
Try increase coverage for issue 141
serges147 Apr 14, 2025
58ea208
Try increase coverage for issue 141
serges147 Apr 14, 2025
cc473cf
Try increase coverage for issue 141
serges147 Apr 14, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions CONTRIBUTING.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,13 +67,13 @@ and manually run it.

### TLDR

See [devcontainer.json](.devcontainer/toolshed/devcontainer.json) for the current value of x for `ts22.4.x`.
See [devcontainer.json](.devcontainer/toolshed/devcontainer.json) for the current value of x for `ts24.4.x`.

```
docker pull ghcr.io/opencyphal/toolshed:ts22.4.x
docker pull ghcr.io/opencyphal/toolshed:ts24.4.x
git clone {this repo}
cd CETL
docker run --rm -it -v ${PWD}:/repo ghcr.io/opencyphal/toolshed:ts22.4.x
docker run --rm -it -v ${PWD}:/repo ghcr.io/opencyphal/toolshed:ts24.4.x
cd cetlvast
cmake --workflow --preset workflow-clang-native-cpp-14-online
```
Expand All @@ -82,7 +82,7 @@ cmake --workflow --preset workflow-clang-native-cpp-14-online

1. Pull the OpenCyphal dev-container used for CETL:
```
docker pull ghcr.io/opencyphal/toolshed:ts22.4.x
docker pull ghcr.io/opencyphal/toolshed:ts24.4.x
```
2. Clone CETL, cd into the repo, and launch an interactive terminal session of
the dev container. This command will mount the current directory (`${PWD}`) in
Expand All @@ -93,7 +93,7 @@ building.
```
git clone {this repo}
cd CETL
docker run --rm -it -v ${PWD}:/repo ghcr.io/opencyphal/toolshed:ts22.4.x
docker run --rm -it -v ${PWD}:/repo ghcr.io/opencyphal/toolshed:ts24.4.x
```
3. See available configurations
```
Expand All @@ -120,7 +120,7 @@ To ensure that the formatting matches the expectations of the CI suite,
invoke Clang-Format of the correct version from the container (be sure to use the correct image tag):

```
docker run --rm -v ${PWD}:/repo ghcr.io/opencyphal/toolshed:ts22.4.x /bin/sh -c 'cd cetlvast && cmake --preset configure-clang-native-cpp-17-offline && cd build && ninja' danger-danger-cetlvast-clang-format-in-place'
docker run --rm -v ${PWD}:/repo ghcr.io/opencyphal/toolshed:ts24.4.x /bin/sh -c 'cd cetlvast && cmake --preset configure-clang-native-cpp-17-offline && cd build && ninja danger-danger-cetlvast-clang-format-in-place'
```

# `issue/*` and hashtag-based CI triggering
Expand Down
148 changes: 117 additions & 31 deletions cetlvast/suites/unittest/test_unbounded_variant.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,7 @@ struct side_effect_stats

struct MyBase : rtti_helper<type_id_type<0x1, 0x0>>
{
char payload_;
char payload_{};
int value_ = 0;
bool moved_ = false;

Expand Down Expand Up @@ -300,7 +300,7 @@ class TestPmrUnboundedVariant : public testing::Test
return &mr_;
}

pmr* get_default_mr() noexcept
static pmr* get_default_mr() noexcept
{
return cetl::pmr::get_default_resource();
}
Expand Down Expand Up @@ -334,12 +334,12 @@ TEST_F(TestPmrUnboundedVariant, bad_unbounded_variant_access_assignment)
#if defined(__cpp_exceptions)

// Test the copy assignment operator.
cetl::bad_unbounded_variant_access test_exception1;
cetl::bad_unbounded_variant_access test_exception2;
const cetl::bad_unbounded_variant_access test_exception1{};
cetl::bad_unbounded_variant_access test_exception2{};
test_exception2 = test_exception1;

// Test the move assignment operator.
cetl::bad_unbounded_variant_access test_exception3;
cetl::bad_unbounded_variant_access test_exception3{};
test_exception3 = std::move(test_exception2);
EXPECT_THAT(test_exception3.what(), "bad unbounded variant access");

Expand Down Expand Up @@ -438,7 +438,9 @@ TEST_F(TestPmrUnboundedVariant, ctor_2_copy)
EXPECT_THAT(dst.type_id(), type_id_value<int>);

EXPECT_THAT(get<int>(src), 42);
EXPECT_THAT(get_if<char>(&src), IsNull());
EXPECT_THAT(get<int>(dst), 42);
EXPECT_THAT(get_if<char>(&dst), IsNull());

const ub_var empty{};
EXPECT_THAT(empty.type_size(), 0);
Expand All @@ -458,10 +460,10 @@ TEST_F(TestPmrUnboundedVariant, ctor_2_copy)
const ub_var src{test{}};
ub_var dst{src};

EXPECT_THAT(get<test>(src).value_, 1 + 10);
EXPECT_THAT(get<test>(src).value_, 1);
EXPECT_THAT(get<const test&>(src).value_, 1);

EXPECT_THAT(get<test>(dst).value_, 1 + 10 + 10);
EXPECT_THAT(get<test>(dst).value_, 1 + 10);
EXPECT_THAT(get<test&>(dst).value_, 1 + 10);
EXPECT_THAT(get<const test&>(dst).value_, 1 + 10);

Expand Down Expand Up @@ -512,7 +514,7 @@ TEST_F(TestPmrUnboundedVariant, ctor_2_copy)
using ub_var = unbounded_variant<sizeof(test), false>;

ub_var src{test{}};
EXPECT_THAT(get<test>(src).value_, 1 + 10);
EXPECT_THAT(get<test>(src).value_, 1);
EXPECT_THAT(get<test&>(src).value_, 1);
EXPECT_THAT(get<test>(std::move(src)).value_, 1 + 1);
// const ub_var dst{src}; //< expectedly won't compile (due to !copyable `unbounded_variant`)
Expand Down Expand Up @@ -611,7 +613,7 @@ TEST_F(TestPmrUnboundedVariant, ctor_5_in_place)

const ub_var src{ub_var::in_place_type_t<MyType>{}, 'Y', 42};

const auto test = get<MyType>(src);
const auto& test = get<MyType>(src);
EXPECT_THAT(test.ch_, 'Y');
EXPECT_THAT(test.number_, 42);
}
Expand All @@ -633,7 +635,7 @@ TEST_F(TestPmrUnboundedVariant, ctor_6_in_place_initializer_list)

const ub_var src{ub_var::in_place_type_t<MyType>{}, {'A', 'B', 'C'}, 42};

auto& test = get<const MyType&>(src);
const auto& test = get<const MyType&>(src);
EXPECT_THAT(test.size_, 3);
EXPECT_THAT(test.number_, 42);
}
Expand Down Expand Up @@ -697,8 +699,8 @@ TEST_F(TestPmrUnboundedVariant, assign_1_copy)
dst = src2;
EXPECT_THAT(stats.ops, "@CC@C~C");

auto dst_ptr = &dst;
dst = *dst_ptr;
const auto* const dst_ptr = &dst;
dst = *dst_ptr;
EXPECT_THAT(stats.ops, "@CC@C~C");

EXPECT_THAT(get<const test&>(src2).value_, 10);
Expand Down Expand Up @@ -730,8 +732,8 @@ TEST_F(TestPmrUnboundedVariant, assign_2_move)
dst = ub_var{147};
EXPECT_THAT(get<int>(dst), 147);

auto dst_ptr = &dst;
dst = std::move(*dst_ptr);
auto* const dst_ptr = &dst;
dst = std::move(*dst_ptr);
EXPECT_THAT(get<int>(dst), 147);

dst = ub_var{};
Expand Down Expand Up @@ -839,7 +841,7 @@ TEST_F(TestPmrUnboundedVariant, make_unbounded_variant_2_list)
using ub_var = unbounded_variant<sizeof(MyType)>;

const auto src = make_unbounded_variant<MyType, ub_var>({'A', 'C'}, 42);
const auto& test = get<const MyType&>(src);
const auto& test = get<MyType>(src);
EXPECT_THAT(test.size_, 2);
EXPECT_THAT(test.number_, 42);

Expand Down Expand Up @@ -874,7 +876,7 @@ TEST_F(TestPmrUnboundedVariant, get_cppref_example)
ra[1] = 'o';
EXPECT_THAT(get<const std::string&>(a1), "hollo"); //< const reference

auto s1 = get<std::string&&>(std::move(a1)); //< rvalue reference
auto s1 = get<std::string>(std::move(a1)); //< rvalue reference
// Note: `s1` is a move-constructed std::string, `a1` is empty
static_assert(std::is_same<decltype(s1), std::string>::value, "");
EXPECT_THAT(s1, "hollo");
Expand Down Expand Up @@ -1133,8 +1135,8 @@ TEST_F(TestPmrUnboundedVariant, emplace_1)
{
using ub_var = unbounded_variant<sizeof(char)>;

ub_var src;
auto y_ptr = src.emplace<char>('Y');
ub_var src;
const auto y_ptr = src.emplace<char>('Y');
EXPECT_THAT(get_if<char>(&src), y_ptr);
EXPECT_THAT(get<char>(src), 'Y');
}
Expand All @@ -1146,16 +1148,16 @@ TEST_F(TestPmrUnboundedVariant, emplace_1)
char ch_;
int number_;

MyType(char ch, int number)
MyType(const char ch, const int number)
{
ch_ = ch;
number_ = number;
}
};
using ub_var = unbounded_variant<sizeof(MyType)>;

ub_var t;
auto my_ptr = t.emplace<MyType>('Y', 147);
ub_var t;
const auto my_ptr = t.emplace<MyType>('Y', 147);
EXPECT_THAT(get_if<MyType>(&t), my_ptr);
EXPECT_THAT(get<MyType>(t).ch_, 'Y');
EXPECT_THAT(get<MyType>(t).number_, 147);
Expand All @@ -1169,7 +1171,7 @@ TEST_F(TestPmrUnboundedVariant, emplace_1_ctor_exception)
using ub_var = unbounded_variant<sizeof(MyCopyableAndMovable)>;

auto stats_side_effects = stats.make_side_effect_fn();
auto throwing_side_effects = [=](side_effect_op op) {
auto throwing_side_effects = [=](const side_effect_op op) {
stats_side_effects(op);
if (op == side_effect_op::Construct)
{
Expand Down Expand Up @@ -1296,6 +1298,7 @@ TEST_F(TestPmrUnboundedVariant, pmr_ctor_with_footprint)
dst2 = int{-1};
EXPECT_THAT(dst2.has_value(), true);
EXPECT_THAT(get<int>(dst2), -1);
EXPECT_THAT(get<int>(static_cast<const ub_var&>(dst2)), -1);

ub_var dst3{std::move(dst2)};
EXPECT_THAT(dst3.get_memory_resource(), get_mr());
Expand All @@ -1306,6 +1309,10 @@ TEST_F(TestPmrUnboundedVariant, pmr_ctor_with_footprint)
EXPECT_THAT(dst3.has_value(), true);
EXPECT_THAT(get<bool>(dst3), true);

const ub_var dst3b{dst3};
EXPECT_THAT(dst3b.has_value(), true);
EXPECT_THAT(get<bool>(dst3b), true);

const ub_var src_empty{get_mr()};
ub_var dst4{src_empty};
EXPECT_THAT(dst4.has_value(), false);
Expand Down Expand Up @@ -1378,6 +1385,21 @@ TEST_F(TestPmrUnboundedVariant, pmr_ctor_no_footprint)
EXPECT_THAT(dst5.get_memory_resource(), get_default_mr());
}

TEST_F(TestPmrUnboundedVariant, pmr_ctor_no_footprint_no_memory)
{
using ub_var = unbounded_variant<0 /*Footprint*/, true /*Copyable*/, true /*Movable*/, 2 /*Alignment*/, pmr>;

StrictMock<cetlvast::MemoryResourceMock> mr_mock{};
EXPECT_CALL(mr_mock, do_allocate(_, _)).WillOnce(Return(nullptr));

#if defined(__cpp_exceptions)
EXPECT_THROW(sink(ub_var{mr_mock.resource(), 'x'}), std::bad_alloc);
#else
const ub_var test{mr_mock.resource(), 'x'};
EXPECT_THAT(test.has_value(), false);
#endif
}

TEST_F(TestPmrUnboundedVariant, pmr_with_footprint_move_value_when_out_of_memory)
{
using ub_var = unbounded_variant<2 /*Footprint*/, false /*Copyable*/, true /*Movable*/, 4 /*Alignment*/, pmr>;
Expand Down Expand Up @@ -1411,7 +1433,7 @@ TEST_F(TestPmrUnboundedVariant, pmr_with_footprint_move_value_when_out_of_memory
EXPECT_THAT(dst.valueless_by_exception(), false);
}

// Assign even bigger (`double`) type value which requires more than 2 bytes.
// Assign an even bigger (`double`) type value which requires more than 2 bytes.
// Emulate that there is no memory enough.
{
MyMovableOnly my_move_only{'X', side_effects};
Expand All @@ -1436,8 +1458,8 @@ TEST_F(TestPmrUnboundedVariant, pmr_with_footprint_move_value_when_out_of_memory

TEST_F(TestPmrUnboundedVariant, pmr_with_footprint_copy_value_when_out_of_memory)
{
const auto Alignment = alignof(std::max_align_t);
using ub_var = unbounded_variant<2 /*Footprint*/, true /*Copyable*/, false /*Movable*/, Alignment, pmr>;
constexpr auto Alignment = alignof(std::max_align_t);
using ub_var = unbounded_variant<2 /*Footprint*/, true /*Copyable*/, false /*Movable*/, Alignment, pmr>;

side_effect_stats stats;
auto side_effects = stats.make_side_effect_fn();
Expand Down Expand Up @@ -1498,8 +1520,8 @@ TEST_F(TestPmrUnboundedVariant, pmr_with_footprint_copy_value_when_out_of_memory

TEST_F(TestPmrUnboundedVariant, pmr_no_footprint_move_value_when_out_of_memory)
{
const auto Alignment = alignof(std::max_align_t);
using ub_var = unbounded_variant<0 /*Footprint*/, false /*Copyable*/, true /*Movable*/, Alignment, pmr>;
constexpr auto Alignment = alignof(std::max_align_t);
using ub_var = unbounded_variant<0 /*Footprint*/, false /*Copyable*/, true /*Movable*/, Alignment, pmr>;

side_effect_stats stats;
auto side_effects = stats.make_side_effect_fn();
Expand Down Expand Up @@ -1530,8 +1552,8 @@ TEST_F(TestPmrUnboundedVariant, pmr_no_footprint_move_value_when_out_of_memory)

TEST_F(TestPmrUnboundedVariant, pmr_no_footprint_copy_value_when_out_of_memory)
{
const auto Alignment = alignof(std::max_align_t);
using ub_var = unbounded_variant<0 /*Footprint*/, true /*Copyable*/, false /*Movable*/, Alignment, pmr>;
constexpr auto Alignment = alignof(std::max_align_t);
using ub_var = unbounded_variant<0 /*Footprint*/, true /*Copyable*/, false /*Movable*/, Alignment, pmr>;

side_effect_stats stats;
auto side_effects = stats.make_side_effect_fn();
Expand Down Expand Up @@ -1727,8 +1749,8 @@ TEST_F(TestPmrUnboundedVariant, pmr_use_mock_as_custom_mr_type)
using custom_mr_mock = StrictMock<cetlvast::MemoryResourceMock>;
custom_mr_mock mr_mock{};

const auto Alignment = alignof(std::max_align_t);
using ub_var = unbounded_variant<sizeof(int), true, true, Alignment, custom_mr_mock>;
constexpr auto Alignment = alignof(std::max_align_t);
using ub_var = unbounded_variant<sizeof(int), true, true, Alignment, custom_mr_mock>;
static_assert(std::is_same<ub_var::pmr_type, custom_mr_mock>::value, "should be custom memory resource mock");

auto src = make_unbounded_variant<int, ub_var>(&mr_mock, 42);
Expand All @@ -1746,6 +1768,70 @@ TEST_F(TestPmrUnboundedVariant, pmr_use_mock_as_custom_mr_type)
EXPECT_THAT(get<double>(src), 3.1415926);
}

TEST_F(TestPmrUnboundedVariant, get_if_for_both_bounded_and_unbounded_variants)
{
using Var = cetl::variant<int, std::string>;
using UbVar = unbounded_variant<sizeof(int)>;

constexpr int test_value = 42;

// Const
{
const Var var{test_value};
const auto ub_var = make_unbounded_variant<int, UbVar>(test_value);

// Var
{
const int* const pv = cetl::get_if<int>(&var);
ASSERT_THAT(pv, NotNull());
EXPECT_THAT(*pv, test_value);

const int& vr = cetl::get<int>(var);
EXPECT_THAT(vr, test_value);
}
// UbVar
{
const int* pv = cetl::get_if<int>(&ub_var);
ASSERT_THAT(pv, NotNull());
EXPECT_THAT(*pv, test_value);

const int& vr = cetl::get<int>(ub_var);
EXPECT_THAT(vr, test_value);
}
}

// Non-const
{
Var var{test_value};
auto ub_var = make_unbounded_variant<int, UbVar>(test_value);

// Var
{
int* pv = cetl::get_if<int>(&var);
ASSERT_THAT(pv, NotNull());
EXPECT_THAT(*pv, test_value);

int& vr = cetl::get<int>(var);
EXPECT_THAT(vr, test_value);

int v = cetl::get<int>(std::move(var));
EXPECT_THAT(v, test_value);
}
// UbVar
{
int* pv = cetl::get_if<int>(&ub_var);
ASSERT_THAT(pv, NotNull());
EXPECT_THAT(*pv, test_value);

int& vr = cetl::get<int>(ub_var);
EXPECT_THAT(vr, test_value);

int v = cetl::get<int>(std::move(ub_var));
EXPECT_THAT(v, test_value);
}
}
}

} // namespace

namespace cetl
Expand Down
Loading
Loading