Skip to content

Commit

Permalink
[#38] Never allocate Optional Views on heap
Browse files Browse the repository at this point in the history
* necessary to ensure continuous allocation
  • Loading branch information
Mi-La committed Jan 22, 2025
1 parent 56a59ce commit 7e8a069
Show file tree
Hide file tree
Showing 3 changed files with 92 additions and 32 deletions.
36 changes: 18 additions & 18 deletions runtime/ClangTidySuppressions.txt
Original file line number Diff line number Diff line change
Expand Up @@ -8,12 +8,12 @@ bugprone-signed-char-misuse:src/zserio/DeltaContext.h:257
bugprone-signed-char-misuse:src/zserio/DeltaContext.h:273

# Optional is a generic container and exceptions may be thrown from the value type.
bugprone-exception-escape:src/zserio/Optional.h:352
bugprone-exception-escape:src/zserio/Optional.h:379
bugprone-exception-escape:src/zserio/Optional.h:357
bugprone-exception-escape:src/zserio/Optional.h:384

# False positives - has_value() is checked before value().
bugprone-exception-escape:src/zserio/Optional.h:215
bugprone-exception-escape:src/zserio/Optional.h:399
bugprone-exception-escape:src/zserio/Optional.h:220
bugprone-exception-escape:src/zserio/Optional.h:404

# Variant is a generic container and exceptions may be thrown from the element types.
bugprone-exception-escape:src/zserio/Variant.h:242
Expand All @@ -27,7 +27,7 @@ bugprone-use-after-move:src/zserio/Variant.h:681
cert-dcl58-cpp:src/zserio/BitBuffer.h:587
cert-dcl58-cpp:src/zserio/BitBuffer.h:599
cert-dcl58-cpp:src/zserio/Extended.h:203
cert-dcl58-cpp:src/zserio/Optional.h:1165
cert-dcl58-cpp:src/zserio/Optional.h:1170
cert-dcl58-cpp:src/zserio/Variant.h:841
cert-dcl58-cpp:src/zserio/View.h:223

Expand All @@ -46,14 +46,14 @@ cppcoreguidelines-macro-usage:src/zserio/CppRuntimeVersion.h:8
cppcoreguidelines-pro-bounds-array-to-pointer-decay:src/zserio/CppRuntimeException.h:193

# Optional follows std::optional so passing value by const & is correct.
cppcoreguidelines-pro-bounds-array-to-pointer-decay:src/zserio/Optional.h:577
cppcoreguidelines-pro-bounds-array-to-pointer-decay:src/zserio/Optional.h:594
cppcoreguidelines-pro-bounds-array-to-pointer-decay:src/zserio/Optional.h:794
cppcoreguidelines-pro-bounds-array-to-pointer-decay:src/zserio/Optional.h:808
cppcoreguidelines-pro-bounds-array-to-pointer-decay:src/zserio/Optional.h:934
cppcoreguidelines-pro-bounds-array-to-pointer-decay:src/zserio/Optional.h:948
cppcoreguidelines-pro-bounds-array-to-pointer-decay:src/zserio/Optional.h:1006
cppcoreguidelines-pro-bounds-array-to-pointer-decay:src/zserio/Optional.h:1020
cppcoreguidelines-pro-bounds-array-to-pointer-decay:src/zserio/Optional.h:582
cppcoreguidelines-pro-bounds-array-to-pointer-decay:src/zserio/Optional.h:599
cppcoreguidelines-pro-bounds-array-to-pointer-decay:src/zserio/Optional.h:799
cppcoreguidelines-pro-bounds-array-to-pointer-decay:src/zserio/Optional.h:813
cppcoreguidelines-pro-bounds-array-to-pointer-decay:src/zserio/Optional.h:939
cppcoreguidelines-pro-bounds-array-to-pointer-decay:src/zserio/Optional.h:953
cppcoreguidelines-pro-bounds-array-to-pointer-decay:src/zserio/Optional.h:1011
cppcoreguidelines-pro-bounds-array-to-pointer-decay:src/zserio/Optional.h:1025

# This is necessary for low level implementation of Span to mimic standard C++20 'std::span' abstraction.
cppcoreguidelines-pro-bounds-array-to-pointer-decay:src/zserio/Span.h:113
Expand Down Expand Up @@ -85,10 +85,10 @@ cppcoreguidelines-pro-type-reinterpret-cast:src/zserio/FileUtil.cpp:19
cppcoreguidelines-pro-type-reinterpret-cast:src/zserio/FileUtil.cpp:49

# Optional follows std::optional so the ctors should be implicit.
google-explicit-constructor:src/zserio/Optional.h:117
google-explicit-constructor:src/zserio/Optional.h:127
google-explicit-constructor:src/zserio/Optional.h:137
google-explicit-constructor:src/zserio/Optional.h:238
google-explicit-constructor:src/zserio/Optional.h:122
google-explicit-constructor:src/zserio/Optional.h:132
google-explicit-constructor:src/zserio/Optional.h:142
google-explicit-constructor:src/zserio/Optional.h:243

# This is necessary for low level implementation of Span to mimic standard C++20 'std::span' abstraction.
google-explicit-constructor:src/zserio/Span.h:112
Expand All @@ -110,7 +110,7 @@ google-readability-casting:src/zserio/Variant.h:66
google-explicit-constructor:src/zserio/View.h:133

# Intentional tests.
bugprone-exception-escape:test/zserio/OptionalTest.cpp:284
bugprone-exception-escape:test/zserio/OptionalTest.cpp:75
bugprone-exception-escape:test/zserio/VariantTest.cpp:35
cppcoreguidelines-avoid-c-arrays:test/zserio/SpanTest.cpp:45
cppcoreguidelines-special-member-functions:test/zserio/VariantTest.cpp:32
Expand Down
5 changes: 5 additions & 0 deletions runtime/src/zserio/Optional.h
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
#include "zserio/CppRuntimeException.h"
#include "zserio/HashCodeUtil.h"
#include "zserio/Traits.h"
#include "zserio/View.h"

namespace zserio
{
Expand Down Expand Up @@ -45,6 +46,10 @@ template <typename T>
struct is_optional_heap_allocated : is_optional_heap_allocated_impl<T>
{};

template <typename T>
struct is_optional_heap_allocated<View<T>> : std::false_type
{};

template <typename T>
constexpr bool is_optional_heap_allocated_v = is_optional_heap_allocated<T>::value;

Expand Down
83 changes: 69 additions & 14 deletions runtime/test/zserio/OptionalTest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
#include "zserio/Optional.h"
#include "zserio/TrackingAllocator.h"
#include "zserio/Types.h"
#include "zserio/View.h"

namespace zserio
{
Expand Down Expand Up @@ -47,6 +48,53 @@ struct BigObj
std::array<char, 30> data;
};

template <>
class View<BigObj>
{
public:
explicit View(const BigObj& data, std::array<char, 30> param) :
m_data(data),
m_param(param)
{}

const std::array<char, 30>& param() const
{
return m_param;
}

const BigObj& zserioData() const
{
return m_data;
}

private:
const BigObj& m_data;
std::array<char, 30> m_param;
};

struct Recursive
{
using IS_RECURSIVE = int;
Optional<Recursive> next;
};

template <>
class View<Recursive>
{
public:
explicit View(const Recursive& data) :
m_data(data)
{}

const Recursive& zserioData() const
{
return m_data;
}

private:
const Recursive& m_data;
};

template <class ALLOC>
class OptionalTest : public testing::Test
{
Expand Down Expand Up @@ -279,22 +327,29 @@ TYPED_TEST(OptionalTest, optionalInOptional)
ASSERT_EQ("test", stringOpt.value().value());
}

TYPED_TEST(OptionalTest, bigObjOpt)
{
ASSERT_TRUE(detail::is_optional_heap_allocated_v<BigObj>);
ASSERT_FALSE(detail::is_optional_heap_allocated_v<View<BigObj>>);

Optional<BigObj> opt;
opt = BigObj();
ASSERT_TRUE(opt.has_value());
ASSERT_TRUE(std::as_const(opt).has_value());
}

TYPED_TEST(OptionalTest, recursiveOpt)
{
struct Tmp
{
using IS_RECURSIVE = int;
Optional<Tmp> next;
};
ASSERT_TRUE(detail::is_optional_heap_allocated_v<Tmp>);
Optional<Tmp> opt1;
opt1 = Tmp();
ASSERT_TRUE(opt1.has_value());
ASSERT_TRUE(!opt1->next.has_value());
ASSERT_TRUE(!std::as_const(opt1)->next.has_value());
opt1->next = Tmp();
ASSERT_TRUE(opt1->next.has_value());
ASSERT_TRUE(!opt1->next->next.has_value());
ASSERT_TRUE(detail::is_optional_heap_allocated_v<Recursive>);
ASSERT_FALSE(detail::is_optional_heap_allocated_v<View<Recursive>>);
Optional<Recursive> opt;
opt = Recursive();
ASSERT_TRUE(opt.has_value());
ASSERT_TRUE(!opt->next.has_value());
ASSERT_TRUE(!std::as_const(opt)->next.has_value());
opt->next = Recursive();
ASSERT_TRUE(opt->next.has_value());
ASSERT_TRUE(!opt->next->next.has_value());
}

TYPED_TEST(OptionalTest, errors)
Expand Down

0 comments on commit 7e8a069

Please sign in to comment.