From 1e0466c4ab26e82abed7f8f263dfe6a2a543cc1a Mon Sep 17 00:00:00 2001
From: Rob Hughes <robert.hughes@arm.com>
Date: Wed, 11 Sep 2019 09:51:13 +0100
Subject: Add "explicit" qualifier to Optional -> bool conversion method

This prevents unintended conversions that could lead to incorrect code
compiling, e.g. Optional<a> == Optional<b>

Also add comparison (==) operator to compare two Optionals.

Update unit tests accordingly

Change-Id: I6f975de7e666ba1ffe16c3ab50643116c6317135
Signed-off-by: Rob Hughes <robert.hughes@arm.com>
---
 include/armnn/Optional.hpp      | 20 +++++++++++++++++++-
 src/armnn/test/OptionalTest.cpp | 26 +++++++++++++++-----------
 2 files changed, 34 insertions(+), 12 deletions(-)

diff --git a/include/armnn/Optional.hpp b/include/armnn/Optional.hpp
index 29be3829a7..863b122716 100644
--- a/include/armnn/Optional.hpp
+++ b/include/armnn/Optional.hpp
@@ -55,7 +55,10 @@ public:
         return m_HasValue;
     }
 
-    operator bool() const noexcept
+    /// Conversion to bool, so can be used in if-statements and similar contexts expecting a bool.
+    /// Note this is explicit so that it doesn't get implicitly converted to a bool in unwanted cases,
+    /// for example "Optional<TypeA> == Optional<TypeB>" should not compile.
+    explicit operator bool() const noexcept
     {
         return has_value();
     }
@@ -278,6 +281,21 @@ public:
     template<class... Args>
     explicit Optional(ConstructInPlace, Args&&... args) :
         BaseSwitch(CONSTRUCT_IN_PLACE, std::forward<Args>(args)...) {}
+
+    /// Two optionals are considered equal if they are both empty or both contain values which
+    /// themselves are considered equal (via their own == operator).
+    bool operator==(const Optional<T>& rhs) const
+    {
+        if (!this->has_value() && !rhs.has_value())
+        {
+            return true;
+        }
+        if (this->has_value() && rhs.has_value() && this->value() == rhs.value())
+        {
+            return true;
+        }
+        return false;
+    }
 };
 
 // Utility template that constructs an object of type T in-place and wraps
diff --git a/src/armnn/test/OptionalTest.cpp b/src/armnn/test/OptionalTest.cpp
index e2054399f1..f36ab34682 100644
--- a/src/armnn/test/OptionalTest.cpp
+++ b/src/armnn/test/OptionalTest.cpp
@@ -25,31 +25,33 @@ BOOST_AUTO_TEST_SUITE(OptionalTests)
 BOOST_AUTO_TEST_CASE(SimpleStringTests)
 {
     armnn::Optional<std::string> optionalString;
-    BOOST_TEST(optionalString == false);
+    BOOST_TEST(static_cast<bool>(optionalString) == false);
     BOOST_TEST(optionalString.has_value() == false);
+    BOOST_TEST((optionalString == armnn::Optional<std::string>()));
 
     optionalString = std::string("Hello World");
-    BOOST_TEST(optionalString == true);
+    BOOST_TEST(static_cast<bool>(optionalString) == true);
     BOOST_TEST(optionalString.has_value() == true);
     BOOST_TEST(optionalString.value() == "Hello World");
+    BOOST_TEST((optionalString == armnn::Optional<std::string>("Hello World")));
 
     armnn::Optional<std::string> otherString;
     otherString = optionalString;
-    BOOST_TEST(otherString == true);
+    BOOST_TEST(static_cast<bool>(otherString) == true);
     BOOST_TEST(otherString.value() == "Hello World");
 
     optionalString.reset();
-    BOOST_TEST(optionalString == false);
+    BOOST_TEST(static_cast<bool>(optionalString) == false);
     BOOST_TEST(optionalString.has_value() == false);
 
     const std::string stringValue("Hello World");
     armnn::Optional<std::string> optionalString2(stringValue);
-    BOOST_TEST(optionalString2 == true);
+    BOOST_TEST(static_cast<bool>(optionalString2) == true);
     BOOST_TEST(optionalString2.has_value() == true);
     BOOST_TEST(optionalString2.value() == "Hello World");
 
     armnn::Optional<std::string> optionalString3(std::move(optionalString2));
-    BOOST_TEST(optionalString3 == true);
+    BOOST_TEST(static_cast<bool>(optionalString3) == true);
     BOOST_TEST(optionalString3.has_value() == true);
     BOOST_TEST(optionalString3.value() == "Hello World");
 }
@@ -96,17 +98,19 @@ BOOST_AUTO_TEST_CASE(SimpleIntTests)
     const int intValue = 123;
 
     armnn::Optional<int> optionalInt;
-    BOOST_TEST(optionalInt == false);
+    BOOST_TEST(static_cast<bool>(optionalInt) == false);
     BOOST_TEST(optionalInt.has_value() == false);
+    BOOST_TEST((optionalInt == armnn::Optional<int>()));
 
     optionalInt = intValue;
-    BOOST_TEST(optionalInt == true);
+    BOOST_TEST(static_cast<bool>(optionalInt) == true);
     BOOST_TEST(optionalInt.has_value() == true);
     BOOST_TEST(optionalInt.value() == intValue);
+    BOOST_TEST((optionalInt == armnn::Optional<int>(intValue)));
 
     armnn::Optional<int> otherOptionalInt;
     otherOptionalInt = optionalInt;
-    BOOST_TEST(otherOptionalInt == true);
+    BOOST_TEST(static_cast<bool>(otherOptionalInt) == true);
     BOOST_TEST(otherOptionalInt.value() == intValue);
 }
 
@@ -137,13 +141,13 @@ BOOST_AUTO_TEST_CASE(ObjectConstructedInPlaceTests)
 
     // Use MakeOptional
     armnn::Optional<SimpleObject> optionalObject1 = armnn::MakeOptional<SimpleObject>(objectName, objectValue);
-    BOOST_CHECK(optionalObject1 == true);
+    BOOST_CHECK(static_cast<bool>(optionalObject1) == true);
     BOOST_CHECK(optionalObject1.has_value() == true);
     BOOST_CHECK(optionalObject1.value() == referenceObject);
 
     // Call in-place constructor directly
     armnn::Optional<SimpleObject> optionalObject2(CONSTRUCT_IN_PLACE, objectName, objectValue);
-    BOOST_CHECK(optionalObject1 == true);
+    BOOST_CHECK(static_cast<bool>(optionalObject1) == true);
     BOOST_CHECK(optionalObject1.has_value() == true);
     BOOST_CHECK(optionalObject1.value() == referenceObject);
 }
-- 
cgit v1.2.1