From 152fb1cf5ef061b21fd3103e97ff8758815d21ea Mon Sep 17 00:00:00 2001 From: David Beck Date: Fri, 5 Oct 2018 15:12:17 +0100 Subject: IVGCVSW-1975 : Remove boost::optional from the public interface IVGCVSW-1964 : Optional to support passing references Change-Id: I4bf8c264d1726aab7e417de5ffd7d04248332e54 --- include/armnn/Optional.hpp | 236 +++++++++++++++++++++++++++++++++------- src/armnn/test/OptionalTest.cpp | 82 ++++++++++++++ 2 files changed, 278 insertions(+), 40 deletions(-) diff --git a/include/armnn/Optional.hpp b/include/armnn/Optional.hpp index 6fc207f425..47afca87b5 100644 --- a/include/armnn/Optional.hpp +++ b/include/armnn/Optional.hpp @@ -5,83 +5,152 @@ #pragma once #include "Exceptions.hpp" +#include +#include + +#include + +// Optional is a drop in replacement for std::optional until we migrate +// to c++-17. Only a subset of the optional features are implemented that +// we intend to use in ArmNN. + +// There are two distinct implementations here: +// +// 1, for normal constructable/destructable types and reference types +// 2, for reference types + +// The std::optional features we support are: +// +// - has_value() and operator bool() to tell if the optional has a value +// - value() returns a reference to the held object +// + +// There is a deprecated and limited support for boost::optional in this class, +// which will be removed in the 19.02 release. namespace armnn { -// NOTE: the members of the Optional class don't follow the ArmNN -// coding convention because the interface to be close to -// the C++-17 interface so we can easily migrate to std::optional -// later. +// EmptyOptional is used to initialize the Optional class in case we want +// to have default value for an Optional in a function declaration. +struct EmptyOptional {}; -template -class Optional final + +// OptionalBase is the common functionality between reference and non-reference +// optional types. +class OptionalBase { public: - Optional(T&& value) - : m_HasValue{true} + OptionalBase() noexcept + : m_HasValue{false} { - new (m_Storage) T(value); } - Optional(const T& value) - : m_HasValue{true} + bool has_value() const noexcept { - new (m_Storage) T(value); + return m_HasValue; } - Optional(const Optional& other) - : m_HasValue{false} + operator bool() const noexcept { - *this = other; + return has_value(); } - Optional() noexcept - : m_HasValue{false} +protected: + OptionalBase(bool hasValue) noexcept + : m_HasValue{hasValue} { } - ~Optional() + bool m_HasValue; +}; + +// +// The default implementation is the non-reference case. This +// has an unsigned char array for storing the optional value which +// is in-place constructed there. +// +template +class OptionalReferenceSwitch : public OptionalBase +{ +public: + using Base = OptionalBase; + + OptionalReferenceSwitch() noexcept : Base{} {} + OptionalReferenceSwitch(EmptyOptional) noexcept : Base{} {} + + OptionalReferenceSwitch(const T& value) + : Base{} { - reset(); + Construct(value); } - operator bool() const noexcept + OptionalReferenceSwitch(const OptionalReferenceSwitch& other) + : Base{} { - return has_value(); + *this = other; + } + + // temporary support for limited conversion from boost + OptionalReferenceSwitch(const boost::optional& other) + : Base{} + { + *this = other; } - Optional& operator=(T&& value) + OptionalReferenceSwitch& operator=(const T& value) { reset(); - new (m_Storage) T(value); - m_HasValue = true; + Construct(value); return *this; } - Optional& operator=(const T& value) + OptionalReferenceSwitch& operator=(const OptionalReferenceSwitch& other) { reset(); - new(m_Storage) T(value); - m_HasValue = true; + if (other.has_value()) + { + Construct(other.value()); + } + return *this; } - Optional& operator=(const Optional& other) + // temporary support for limited conversion from boost + OptionalReferenceSwitch& operator=(const boost::optional& other) { reset(); - if (other.has_value()) + if (other.is_initialized()) { - new (m_Storage) T(other.value()); - m_HasValue = true; + Construct(other.get()); } return *this; } + OptionalReferenceSwitch& operator=(EmptyOptional) + { + reset(); + return *this; + } + + ~OptionalReferenceSwitch() + { + reset(); + } + + void reset() + { + if (Base::has_value()) + { + value().T::~T(); + Base::m_HasValue = false; + } + } + const T& value() const { - if (!has_value()) + if (!Base::has_value()) { throw BadOptionalAccessException("Optional has no value"); } @@ -92,7 +161,7 @@ public: T& value() { - if (!has_value()) + if (!Base::has_value()) { throw BadOptionalAccessException("Optional has no value"); } @@ -101,23 +170,110 @@ public: return *valuePtr; } - bool has_value() const noexcept +private: + void Construct(const T& value) { - return m_HasValue; + new (m_Storage) T(value); + m_HasValue = true; + } + + alignas(alignof(T)) unsigned char m_Storage[sizeof(T)]; +}; + +// +// This is the special case for reference types. This holds a pointer +// to the referenced type. This doesn't own the referenced memory and +// it never calls delete on the pointer. +// +template +class OptionalReferenceSwitch : public OptionalBase +{ +public: + using Base = OptionalBase; + using NonRefT = typename std::remove_reference::type; + + OptionalReferenceSwitch() noexcept : Base{}, m_Storage{nullptr} {} + OptionalReferenceSwitch(EmptyOptional) noexcept : Base{}, m_Storage{nullptr} {} + + OptionalReferenceSwitch(const OptionalReferenceSwitch& other) : Base{} + { + *this = other; + } + + OptionalReferenceSwitch(T value) + : Base{true} + , m_Storage{&value} + { + } + + OptionalReferenceSwitch& operator=(const T value) + { + m_Storage = &value; + Base::m_HasValue = true; + return *this; + } + + OptionalReferenceSwitch& operator=(const OptionalReferenceSwitch& other) + { + m_Storage = other.m_Storage; + Base::m_HasValue = other.has_value(); + return *this; + } + + OptionalReferenceSwitch& operator=(EmptyOptional) + { + reset(); + return *this; + } + + ~OptionalReferenceSwitch() + { + reset(); } void reset() { - if (has_value()) + Base::m_HasValue = false; + m_Storage = nullptr; + } + + const T value() const + { + if (!Base::has_value()) { - value().T::~T(); - m_HasValue = false; + throw BadOptionalAccessException("Optional has no value"); + } + + return *m_Storage; + } + + T value() + { + if (!Base::has_value()) + { + throw BadOptionalAccessException("Optional has no value"); } + + return *m_Storage; } private: - alignas(alignof(T)) unsigned char m_Storage[sizeof(T)]; - bool m_HasValue; + NonRefT* m_Storage; +}; + +template +class Optional final : public OptionalReferenceSwitch::value, T> +{ +public: + using BaseSwitch = OptionalReferenceSwitch::value, T>; + + Optional(const T& value) : BaseSwitch{value} {} + Optional() noexcept : BaseSwitch{} {} + Optional(EmptyOptional empty) : BaseSwitch{empty} {} + Optional(const Optional& other) : BaseSwitch{other} {} + + // temporary support for limited conversion from boost + Optional(const boost::optional& other) : BaseSwitch{other} {} }; } diff --git a/src/armnn/test/OptionalTest.cpp b/src/armnn/test/OptionalTest.cpp index 1b5aaa7db6..87fd156ece 100644 --- a/src/armnn/test/OptionalTest.cpp +++ b/src/armnn/test/OptionalTest.cpp @@ -5,8 +5,34 @@ #include #include +#include #include +namespace +{ + +void PassStringRef(armnn::Optional value) +{ +} + +void PassStringRefWithDefault(armnn::Optional value = armnn::EmptyOptional()) +{ +} + +void BoostCompatibilityTester(const armnn::Optional& optionalString, + bool hasValue, + const std::string& expectedValue) +{ + BOOST_TEST(optionalString.has_value() == hasValue); + if (hasValue) + { + BOOST_TEST(optionalString.value() == expectedValue); + } +} + +} + + BOOST_AUTO_TEST_SUITE(OptionalTests) BOOST_AUTO_TEST_CASE(SimpleStringTests) @@ -41,6 +67,62 @@ BOOST_AUTO_TEST_CASE(SimpleStringTests) BOOST_TEST(optionalString3.value() == "Hello World"); } + +BOOST_AUTO_TEST_CASE(StringRefTests) +{ + armnn::Optional optionalStringRef{armnn::EmptyOptional()}; + BOOST_TEST(optionalStringRef.has_value() == false); + + PassStringRef(optionalStringRef); + PassStringRefWithDefault(); + + armnn::Optional optionalStringRef2 = optionalStringRef; + + std::string helloWorld("Hello World"); + + std::string& helloWorldRef = helloWorld; + armnn::Optional optionalHelloRef = helloWorldRef; + BOOST_TEST(optionalHelloRef.has_value() == true); + BOOST_TEST(optionalHelloRef.value() == "Hello World"); + + armnn::Optional optionalHelloRef2 = helloWorld; + BOOST_TEST(optionalHelloRef2.has_value() == true); + BOOST_TEST(optionalHelloRef2.value() == "Hello World"); + + armnn::Optional optionalHelloRef3{helloWorldRef}; + BOOST_TEST(optionalHelloRef3.has_value() == true); + BOOST_TEST(optionalHelloRef3.value() == "Hello World"); + + armnn::Optional optionalHelloRef4{helloWorld}; + BOOST_TEST(optionalHelloRef4.has_value() == true); + BOOST_TEST(optionalHelloRef4.value() == "Hello World"); + + // modify through the optional reference + optionalHelloRef4.value().assign("Long Other String"); + BOOST_TEST(helloWorld == "Long Other String"); + BOOST_TEST(optionalHelloRef.value() == "Long Other String"); + BOOST_TEST(optionalHelloRef2.value() == "Long Other String"); + BOOST_TEST(optionalHelloRef3.value() == "Long Other String"); +} + +BOOST_AUTO_TEST_CASE(BoostCompatibilityTests) +{ + // sanity checks + BoostCompatibilityTester(armnn::Optional(), false, ""); + BoostCompatibilityTester(armnn::Optional("Hello World"), true, "Hello World"); + + // the real thing is to see that we can pass a boost::optional in place + // of an ArmNN Optional + boost::optional empty; + boost::optional helloWorld("Hello World"); + + BoostCompatibilityTester(empty, false, ""); + BoostCompatibilityTester(helloWorld, true, "Hello World"); + + BoostCompatibilityTester(boost::optional(), false, ""); + BoostCompatibilityTester(boost::optional("Hello World"), true, "Hello World"); +} + BOOST_AUTO_TEST_CASE(SimpleIntTests) { const int intValue = 123; -- cgit v1.2.1