From 807e5b45cc379f795a577377fe5176699437276e Mon Sep 17 00:00:00 2001 From: Chinmay Garde Date: Tue, 12 Mar 2024 12:18:57 -0700 Subject: [PATCH 1/3] [Impeller] Add utility for type safe masks. We use and recommend type safe enums. But when it comes to masks of those enums (`TextureUsageMask`, `ColorWriteMask`, etc.), these are all untyped and you can create one either from a scalar of the enums underlying type, or using ugly and error prone static casts. At the callee, there is no type checking and another error prone static cast. This patch adds a typed safe mask type. This should allow for the migration of something like: ```c++ using TextureUsageMask = uint64_t; ``` with ```c++ using TextureUsageMask = Mask; ``` Subsequently, all static casts can be removed with full type checking throughout. This doesn't migrate existing uses yet. That will come in a separate patch. --- impeller/base/BUILD.gn | 1 + impeller/base/base_unittests.cc | 194 ++++++++++++++++++++++++++++++++ impeller/base/mask.h | 194 ++++++++++++++++++++++++++++++++ 3 files changed, 389 insertions(+) create mode 100644 impeller/base/mask.h diff --git a/impeller/base/BUILD.gn b/impeller/base/BUILD.gn index c021a96e355c0..03ee14f645658 100644 --- a/impeller/base/BUILD.gn +++ b/impeller/base/BUILD.gn @@ -12,6 +12,7 @@ impeller_component("base") { "comparable.cc", "comparable.h", "config.h", + "mask.h", "promise.cc", "promise.h", "strings.cc", diff --git a/impeller/base/base_unittests.cc b/impeller/base/base_unittests.cc index db2c97852be49..d8bf97ce63f26 100644 --- a/impeller/base/base_unittests.cc +++ b/impeller/base/base_unittests.cc @@ -3,6 +3,7 @@ // found in the LICENSE file. #include "flutter/testing/testing.h" +#include "impeller/base/mask.h" #include "impeller/base/promise.h" #include "impeller/base/strings.h" #include "impeller/base/thread.h" @@ -250,5 +251,198 @@ TEST(BaseTest, NoExceptionPromiseEmpty) { wrapper.reset(); } +enum class MyMaskBits : uint32_t { + kFoo = 0, + kBar = 1 << 0, + kBaz = 1 << 1, + kBang = 1 << 2, +}; + +using MyMask = Mask; + +TEST(BaseTest, CanUseTypedMasks) { + { + MyMask mask; + ASSERT_EQ(static_cast(mask), 0u); + ASSERT_FALSE(mask); + } + + { + MyMask mask(MyMaskBits::kBar); + ASSERT_EQ(static_cast(mask), 1u); + ASSERT_TRUE(mask); + } + + { + MyMask mask2(MyMaskBits::kBar); + MyMask mask(mask2); + ASSERT_EQ(static_cast(mask), 1u); + ASSERT_TRUE(mask); + } + + { + MyMask mask2(MyMaskBits::kBar); + MyMask mask(std::move(mask2)); + ASSERT_EQ(static_cast(mask), 1u); + ASSERT_TRUE(mask); + } + + ASSERT_LT(MyMaskBits::kBar, MyMaskBits::kBaz); + ASSERT_LE(MyMaskBits::kBar, MyMaskBits::kBaz); + ASSERT_GT(MyMaskBits::kBaz, MyMaskBits::kBar); + ASSERT_GE(MyMaskBits::kBaz, MyMaskBits::kBar); + ASSERT_EQ(MyMaskBits::kBaz, MyMaskBits::kBaz); + ASSERT_NE(MyMaskBits::kBaz, MyMaskBits::kBang); + + { + MyMask m1(MyMaskBits::kBar); + MyMask m2(MyMaskBits::kBaz); + ASSERT_EQ(static_cast(m1 & m2), 0u); + ASSERT_FALSE(m1 & m2); + } + + { + MyMask m1(MyMaskBits::kBar); + MyMask m2(MyMaskBits::kBaz); + ASSERT_EQ(static_cast(m1 | m2), ((1u << 0u) | (1u << 1u))); + ASSERT_TRUE(m1 | m2); + } + + { + MyMask m1(MyMaskBits::kBar); + MyMask m2(MyMaskBits::kBaz); + ASSERT_EQ(static_cast(m1 ^ m2), ((1u << 0u) ^ (1u << 1u))); + ASSERT_TRUE(m1 ^ m2); + } + + { + MyMask m1(MyMaskBits::kBar); + ASSERT_EQ(static_cast(~m1), (~(1u << 0u))); + ASSERT_TRUE(m1); + } + + { + MyMask m1 = MyMaskBits::kBar; + MyMask m2 = MyMaskBits::kBaz; + m2 = m1; + ASSERT_EQ(m2, MyMaskBits::kBar); + } + + { + MyMask m = MyMaskBits::kBar | MyMaskBits::kBaz; + ASSERT_TRUE(m); + } + + { + MyMask m = MyMaskBits::kBar & MyMaskBits::kBaz; + ASSERT_FALSE(m); + } + + { + MyMask m = MyMaskBits::kBar ^ MyMaskBits::kBaz; + ASSERT_TRUE(m); + } + + { + MyMask m = ~MyMaskBits::kBar; + ASSERT_TRUE(m); + } + + { + MyMask m1 = MyMaskBits::kBar; + MyMask m2 = MyMaskBits::kBaz; + m2 |= m1; + ASSERT_EQ(m1, MyMaskBits::kBar); + MyMask pred = MyMaskBits::kBar | MyMaskBits::kBaz; + ASSERT_EQ(m2, pred); + } + + { + MyMask m1 = MyMaskBits::kBar; + MyMask m2 = MyMaskBits::kBaz; + m2 &= m1; + ASSERT_EQ(m1, MyMaskBits::kBar); + MyMask pred = MyMaskBits::kBar & MyMaskBits::kBaz; + ASSERT_EQ(m2, pred); + } + + { + MyMask m1 = MyMaskBits::kBar; + MyMask m2 = MyMaskBits::kBaz; + m2 ^= m1; + ASSERT_EQ(m1, MyMaskBits::kBar); + MyMask pred = MyMaskBits::kBar ^ MyMaskBits::kBaz; + ASSERT_EQ(m2, pred); + } + + { + MyMask x = MyMaskBits::kBar; + MyMask m = x | MyMaskBits::kBaz; + ASSERT_TRUE(m); + } + + { + MyMask x = MyMaskBits::kBar; + MyMask m = MyMaskBits::kBaz | x; + ASSERT_TRUE(m); + } + + { + MyMask x = MyMaskBits::kBar; + MyMask m = x & MyMaskBits::kBaz; + ASSERT_FALSE(m); + } + + { + MyMask x = MyMaskBits::kBar; + MyMask m = MyMaskBits::kBaz & x; + ASSERT_FALSE(m); + } + + { + MyMask x = MyMaskBits::kBar; + MyMask m = x ^ MyMaskBits::kBaz; + ASSERT_TRUE(m); + } + + { + MyMask x = MyMaskBits::kBar; + MyMask m = MyMaskBits::kBaz ^ x; + ASSERT_TRUE(m); + } + + { + MyMask x = MyMaskBits::kBar; + MyMask m = ~x; + ASSERT_TRUE(m); + } + + { + MyMaskBits x = MyMaskBits::kBar; + MyMask m = MyMaskBits::kBaz; + ASSERT_TRUE(x < m); + ASSERT_TRUE(x <= m); + } + + { + MyMaskBits x = MyMaskBits::kBar; + MyMask m = MyMaskBits::kBaz; + ASSERT_FALSE(x == m); + } + + { + MyMaskBits x = MyMaskBits::kBar; + MyMask m = MyMaskBits::kBaz; + ASSERT_TRUE(x != m); + } + + { + MyMaskBits x = MyMaskBits::kBar; + MyMask m = MyMaskBits::kBaz; + ASSERT_FALSE(x > m); + ASSERT_FALSE(x >= m); + } +} + } // namespace testing } // namespace impeller diff --git a/impeller/base/mask.h b/impeller/base/mask.h new file mode 100644 index 0000000000000..a305b1ccde71f --- /dev/null +++ b/impeller/base/mask.h @@ -0,0 +1,194 @@ +// Copyright 2013 The Flutter Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#ifndef FLUTTER_IMPELLER_BASE_MASK_H_ +#define FLUTTER_IMPELLER_BASE_MASK_H_ + +#include + +namespace impeller { + +//------------------------------------------------------------------------------ +/// @brief A mask of typed enums. +/// +/// @tparam EnumType_ The type of the enum. Must be an enum class. +/// +template +struct Mask { + using EnumType = EnumType_; + using MaskType = typename std::underlying_type::type; + + constexpr Mask() = default; + + constexpr Mask(const Mask& other) = default; + + constexpr Mask(Mask&& other) = default; + + constexpr Mask(EnumType type) : mask_(static_cast(type)) {} + + explicit constexpr Mask(MaskType mask) : mask_(static_cast(mask)) {} + + // All casts must be explicit. + + explicit constexpr operator MaskType() const { return mask_; } + + explicit constexpr operator bool() const { return !!mask_; } + + // The following relational operators can be replaced with a defaulted + // spaceship operator post C++20. + + constexpr bool operator<(const Mask& other) const { + return mask_ < other.mask_; + } + + constexpr bool operator>(const Mask& other) const { + return mask_ > other.mask_; + } + + constexpr bool operator>=(const Mask& other) const { + return mask_ >= other.mask_; + } + + constexpr bool operator<=(const Mask& other) const { + return mask_ <= other.mask_; + } + + constexpr bool operator==(const Mask& other) const { + return mask_ == other.mask_; + } + + constexpr bool operator!=(const Mask& other) const { + return mask_ != other.mask_; + } + + // Logical operators. + + constexpr bool operator!() const { return !mask_; } + + // Bitwise operators. + + constexpr Mask operator&(const Mask& other) const { + return Mask{mask_ & other.mask_}; + } + + constexpr Mask operator|(const Mask& other) const { + return Mask{mask_ | other.mask_}; + } + + constexpr Mask operator^(const Mask& other) const { + return Mask{mask_ ^ other.mask_}; + } + + constexpr Mask operator~() const { return Mask{~mask_}; } + + // Assignment operators. + + constexpr Mask& operator=(const Mask&) = default; + + constexpr Mask& operator=(Mask&&) = default; + + constexpr Mask& operator|=(const Mask& other) { + mask_ |= other.mask_; + return *this; + } + + constexpr Mask& operator&=(const Mask& other) { + mask_ &= other.mask_; + return *this; + } + + constexpr Mask& operator^=(const Mask& other) { + mask_ ^= other.mask_; + return *this; + } + + private: + MaskType mask_ = {}; +}; + +// Construction from Enum Types + +template +inline constexpr Mask operator|(const EnumType& lhs, + const EnumType& rhs) { + return Mask{lhs} | rhs; +} + +template +inline constexpr Mask operator&(const EnumType& lhs, + const EnumType& rhs) { + return Mask{lhs} & rhs; +} + +template +inline constexpr Mask operator^(const EnumType& lhs, + const EnumType& rhs) { + return Mask{lhs} ^ rhs; +} + +template +inline constexpr Mask operator~(const EnumType& other) { + return ~Mask{other}; +} + +template +inline constexpr Mask operator|(const EnumType& lhs, + const Mask& rhs) { + return Mask{lhs} | rhs; +} + +template +inline constexpr Mask operator&(const EnumType& lhs, + const Mask& rhs) { + return Mask{lhs} & rhs; +} + +template +inline constexpr Mask operator^(const EnumType& lhs, + const Mask& rhs) { + return Mask{lhs} ^ rhs; +} + +// Relational operators with EnumType promotion. These can be replaced by a +// defaulted spaceship operator post C++20. + +template +inline constexpr bool operator<(const EnumType& lhs, + const Mask& rhs) { + return Mask{lhs} < rhs; +} + +template +inline constexpr bool operator>(const EnumType& lhs, + const Mask& rhs) { + return Mask{lhs} > rhs; +} + +template +inline constexpr bool operator<=(const EnumType& lhs, + const Mask& rhs) { + return Mask{lhs} <= rhs; +} + +template +inline constexpr bool operator>=(const EnumType& lhs, + const Mask& rhs) { + return Mask{lhs} >= rhs; +} + +template +inline constexpr bool operator==(const EnumType& lhs, + const Mask& rhs) { + return Mask{lhs} == rhs; +} + +template +inline constexpr bool operator!=(const EnumType& lhs, + const Mask& rhs) { + return Mask{lhs} != rhs; +} + +} // namespace impeller + +#endif // FLUTTER_IMPELLER_BASE_MASK_H_ From f1f4dd54100a53585971e66025e6b5b11c5fdf0e Mon Sep 17 00:00:00 2001 From: Chinmay Garde Date: Tue, 12 Mar 2024 13:25:31 -0700 Subject: [PATCH 2/3] Licenses. --- ci/licenses_golden/licenses_flutter | 2 ++ 1 file changed, 2 insertions(+) diff --git a/ci/licenses_golden/licenses_flutter b/ci/licenses_golden/licenses_flutter index 11225ea34e6b4..0b1d1470956b6 100644 --- a/ci/licenses_golden/licenses_flutter +++ b/ci/licenses_golden/licenses_flutter @@ -39503,6 +39503,7 @@ ORIGIN: ../../../flutter/impeller/base/backend_cast.h + ../../../flutter/LICENSE ORIGIN: ../../../flutter/impeller/base/comparable.cc + ../../../flutter/LICENSE ORIGIN: ../../../flutter/impeller/base/comparable.h + ../../../flutter/LICENSE ORIGIN: ../../../flutter/impeller/base/config.h + ../../../flutter/LICENSE +ORIGIN: ../../../flutter/impeller/base/mask.h + ../../../flutter/LICENSE ORIGIN: ../../../flutter/impeller/base/promise.cc + ../../../flutter/LICENSE ORIGIN: ../../../flutter/impeller/base/promise.h + ../../../flutter/LICENSE ORIGIN: ../../../flutter/impeller/base/strings.cc + ../../../flutter/LICENSE @@ -42354,6 +42355,7 @@ FILE: ../../../flutter/impeller/base/backend_cast.h FILE: ../../../flutter/impeller/base/comparable.cc FILE: ../../../flutter/impeller/base/comparable.h FILE: ../../../flutter/impeller/base/config.h +FILE: ../../../flutter/impeller/base/mask.h FILE: ../../../flutter/impeller/base/promise.cc FILE: ../../../flutter/impeller/base/promise.h FILE: ../../../flutter/impeller/base/strings.cc From 209c9c65cadd3136dcb9cc59e2f738c94bf462d2 Mon Sep 17 00:00:00 2001 From: Chinmay Garde Date: Tue, 12 Mar 2024 13:28:15 -0700 Subject: [PATCH 3/3] Lint. --- impeller/base/base_unittests.cc | 2 +- impeller/base/mask.h | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/impeller/base/base_unittests.cc b/impeller/base/base_unittests.cc index d8bf97ce63f26..5653809110607 100644 --- a/impeller/base/base_unittests.cc +++ b/impeller/base/base_unittests.cc @@ -282,7 +282,7 @@ TEST(BaseTest, CanUseTypedMasks) { { MyMask mask2(MyMaskBits::kBar); - MyMask mask(std::move(mask2)); + MyMask mask(std::move(mask2)); // NOLINT ASSERT_EQ(static_cast(mask), 1u); ASSERT_TRUE(mask); } diff --git a/impeller/base/mask.h b/impeller/base/mask.h index a305b1ccde71f..5703c1507c1b9 100644 --- a/impeller/base/mask.h +++ b/impeller/base/mask.h @@ -25,7 +25,8 @@ struct Mask { constexpr Mask(Mask&& other) = default; - constexpr Mask(EnumType type) : mask_(static_cast(type)) {} + constexpr Mask(EnumType type) // NOLINT(google-explicit-constructor) + : mask_(static_cast(type)) {} explicit constexpr Mask(MaskType mask) : mask_(static_cast(mask)) {}