Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Commit 2253387

Browse files
chinmaygardechinmaygarde
authored
[Impeller] Make masks type safe. (#51369)
Uses the utility added in #51361 I counted the removal of 58 static casts. There was one addition made to the original utility however. Vulkan HPP was promoting all enums to its own mask type. This in itself is problematic but we got away with it because there was no one else doing this kind of promotion. Till we added our own utility. To avoid polluting the namespace with methods that may cause ambiguity, enums that are masks must explicitly be marked as maskable with `IMPELLER_ENUM_IS_MASK` in the `impeller` namespace. No change in functionality.
1 parent 06c17f9 commit 2253387

23 files changed

+133
-123
lines changed

‎impeller/base/base_unittests.cc‎

Lines changed: 12 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,18 @@
99
#include "impeller/base/thread.h"
1010

1111
namespace impeller {
12+
13+
enum class MyMaskBits : uint32_t {
14+
kFoo = 0,
15+
kBar = 1 << 0,
16+
kBaz = 1 << 1,
17+
kBang = 1 << 2,
18+
};
19+
20+
using MyMask = Mask<MyMaskBits>;
21+
22+
IMPELLER_ENUM_IS_MASK(MyMaskBits);
23+
1224
namespace testing {
1325

1426
struct Foo {
@@ -251,15 +263,6 @@ TEST(BaseTest, NoExceptionPromiseEmpty) {
251263
wrapper.reset();
252264
}
253265

254-
enum class MyMaskBits : uint32_t {
255-
kFoo = 0,
256-
kBar = 1 << 0,
257-
kBaz = 1 << 1,
258-
kBang = 1 << 2,
259-
};
260-
261-
using MyMask = Mask<MyMaskBits>;
262-
263266
TEST(BaseTest, CanUseTypedMasks) {
264267
{
265268
MyMask mask;

‎impeller/base/mask.h‎

Lines changed: 48 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,21 @@
99

1010
namespace impeller {
1111

12+
template <typename EnumType_>
13+
struct MaskTraits {
14+
static constexpr bool kIsMask = false;
15+
};
16+
17+
//------------------------------------------------------------------------------
18+
/// @brief Declare this in the "impeller" namespace to make the enum
19+
/// maskable.
20+
///
21+
#define IMPELLER_ENUM_IS_MASK(enum_name) \
22+
template <> \
23+
struct MaskTraits<enum_name> { \
24+
static constexpr bool kIsMask = true; \
25+
};
26+
1227
//------------------------------------------------------------------------------
1328
/// @brief A mask of typed enums.
1429
///
@@ -110,42 +125,56 @@ struct Mask {
110125

111126
// Construction from Enum Types
112127

113-
template <typename EnumType>
128+
template <
129+
typename EnumType,
130+
typename std::enable_if<MaskTraits<EnumType>::kIsMask, bool>::type = true>
114131
inline constexpr Mask<EnumType> operator|(const EnumType& lhs,
115132
const EnumType& rhs) {
116133
return Mask<EnumType>{lhs} | rhs;
117134
}
118135

119-
template <typename EnumType>
136+
template <
137+
typename EnumType,
138+
typename std::enable_if<MaskTraits<EnumType>::kIsMask, bool>::type = true>
120139
inline constexpr Mask<EnumType> operator&(const EnumType& lhs,
121140
const EnumType& rhs) {
122141
return Mask<EnumType>{lhs} & rhs;
123142
}
124143

125-
template <typename EnumType>
144+
template <
145+
typename EnumType,
146+
typename std::enable_if<MaskTraits<EnumType>::kIsMask, bool>::type = true>
126147
inline constexpr Mask<EnumType> operator^(const EnumType& lhs,
127148
const EnumType& rhs) {
128149
return Mask<EnumType>{lhs} ^ rhs;
129150
}
130151

131-
template <typename EnumType>
152+
template <
153+
typename EnumType,
154+
typename std::enable_if<MaskTraits<EnumType>::kIsMask, bool>::type = true>
132155
inline constexpr Mask<EnumType> operator~(const EnumType& other) {
133156
return ~Mask<EnumType>{other};
134157
}
135158

136-
template <typename EnumType>
159+
template <
160+
typename EnumType,
161+
typename std::enable_if<MaskTraits<EnumType>::kIsMask, bool>::type = true>
137162
inline constexpr Mask<EnumType> operator|(const EnumType& lhs,
138163
const Mask<EnumType>& rhs) {
139164
return Mask<EnumType>{lhs} | rhs;
140165
}
141166

142-
template <typename EnumType>
167+
template <
168+
typename EnumType,
169+
typename std::enable_if<MaskTraits<EnumType>::kIsMask, bool>::type = true>
143170
inline constexpr Mask<EnumType> operator&(const EnumType& lhs,
144171
const Mask<EnumType>& rhs) {
145172
return Mask<EnumType>{lhs} & rhs;
146173
}
147174

148-
template <typename EnumType>
175+
template <
176+
typename EnumType,
177+
typename std::enable_if<MaskTraits<EnumType>::kIsMask, bool>::type = true>
149178
inline constexpr Mask<EnumType> operator^(const EnumType& lhs,
150179
const Mask<EnumType>& rhs) {
151180
return Mask<EnumType>{lhs} ^ rhs;
@@ -154,37 +183,43 @@ inline constexpr Mask<EnumType> operator^(const EnumType& lhs,
154183
// Relational operators with EnumType promotion. These can be replaced by a
155184
// defaulted spaceship operator post C++20.
156185

157-
template <typename EnumType>
186+
template <typename EnumType,
187+
typename std::enable_if_t<MaskTraits<EnumType>::kIsMask, bool> = true>
158188
inline constexpr bool operator<(const EnumType& lhs,
159189
const Mask<EnumType>& rhs) {
160190
return Mask<EnumType>{lhs} < rhs;
161191
}
162192

163-
template <typename EnumType>
193+
template <typename EnumType,
194+
typename std::enable_if_t<MaskTraits<EnumType>::kIsMask, bool> = true>
164195
inline constexpr bool operator>(const EnumType& lhs,
165196
const Mask<EnumType>& rhs) {
166197
return Mask<EnumType>{lhs} > rhs;
167198
}
168199

169-
template <typename EnumType>
200+
template <typename EnumType,
201+
typename std::enable_if_t<MaskTraits<EnumType>::kIsMask, bool> = true>
170202
inline constexpr bool operator<=(const EnumType& lhs,
171203
const Mask<EnumType>& rhs) {
172204
return Mask<EnumType>{lhs} <= rhs;
173205
}
174206

175-
template <typename EnumType>
207+
template <typename EnumType,
208+
typename std::enable_if_t<MaskTraits<EnumType>::kIsMask, bool> = true>
176209
inline constexpr bool operator>=(const EnumType& lhs,
177210
const Mask<EnumType>& rhs) {
178211
return Mask<EnumType>{lhs} >= rhs;
179212
}
180213

181-
template <typename EnumType>
214+
template <typename EnumType,
215+
typename std::enable_if_t<MaskTraits<EnumType>::kIsMask, bool> = true>
182216
inline constexpr bool operator==(const EnumType& lhs,
183217
const Mask<EnumType>& rhs) {
184218
return Mask<EnumType>{lhs} == rhs;
185219
}
186220

187-
template <typename EnumType>
221+
template <typename EnumType,
222+
typename std::enable_if_t<MaskTraits<EnumType>::kIsMask, bool> = true>
188223
inline constexpr bool operator!=(const EnumType& lhs,
189224
const Mask<EnumType>& rhs) {
190225
return Mask<EnumType>{lhs} != rhs;

‎impeller/core/formats.cc‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -80,13 +80,13 @@ bool Attachment::IsValid() const {
8080

8181
std::string TextureUsageMaskToString(TextureUsageMask mask) {
8282
std::vector<TextureUsage> usages;
83-
if (mask & static_cast<TextureUsageMask>(TextureUsage::kShaderRead)) {
83+
if (mask & TextureUsage::kShaderRead) {
8484
usages.push_back(TextureUsage::kShaderRead);
8585
}
86-
if (mask & static_cast<TextureUsageMask>(TextureUsage::kShaderWrite)) {
86+
if (mask & TextureUsage::kShaderWrite) {
8787
usages.push_back(TextureUsage::kShaderWrite);
8888
}
89-
if (mask & static_cast<TextureUsageMask>(TextureUsage::kRenderTarget)) {
89+
if (mask & TextureUsage::kRenderTarget) {
9090
usages.push_back(TextureUsage::kRenderTarget);
9191
}
9292
std::stringstream stream;

‎impeller/core/formats.h‎

Lines changed: 13 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@
1313

1414
#include "flutter/fml/hash_combine.h"
1515
#include "flutter/fml/logging.h"
16+
#include "impeller/base/mask.h"
1617
#include "impeller/geometry/color.h"
1718
#include "impeller/geometry/rect.h"
1819
#include "impeller/geometry/scalar.h"
@@ -297,18 +298,15 @@ enum class SampleCount : uint8_t {
297298
kCount4 = 4,
298299
};
299300

300-
using TextureUsageMask = uint64_t;
301-
302-
enum class TextureUsage : TextureUsageMask {
301+
enum class TextureUsage {
303302
kUnknown = 0,
304303
kShaderRead = 1 << 0,
305304
kShaderWrite = 1 << 1,
306305
kRenderTarget = 1 << 2,
307306
};
307+
IMPELLER_ENUM_IS_MASK(TextureUsage);
308308

309-
constexpr bool TextureUsageIsRenderTarget(TextureUsageMask mask) {
310-
return static_cast<TextureUsageMask>(TextureUsage::kRenderTarget) & mask;
311-
}
309+
using TextureUsageMask = Mask<TextureUsage>;
312310

313311
constexpr const char* TextureUsageToString(TextureUsage usage) {
314312
switch (usage) {
@@ -435,14 +433,17 @@ enum class SamplerAddressMode {
435433
kDecal,
436434
};
437435

438-
enum class ColorWriteMask : uint64_t {
436+
enum class ColorWriteMaskBits : uint64_t {
439437
kNone = 0,
440438
kRed = 1 << 0,
441439
kGreen = 1 << 1,
442440
kBlue = 1 << 2,
443441
kAlpha = 1 << 3,
444442
kAll = kRed | kGreen | kBlue | kAlpha,
445443
};
444+
IMPELLER_ENUM_IS_MASK(ColorWriteMaskBits);
445+
446+
using ColorWriteMask = Mask<ColorWriteMaskBits>;
446447

447448
constexpr size_t BytesPerPixelForPixelFormat(PixelFormat format) {
448449
switch (format) {
@@ -508,8 +509,7 @@ struct ColorAttachmentDescriptor {
508509
BlendOperation alpha_blend_op = BlendOperation::kAdd;
509510
BlendFactor dst_alpha_blend_factor = BlendFactor::kOneMinusSourceAlpha;
510511

511-
std::underlying_type_t<ColorWriteMask> write_mask =
512-
static_cast<uint64_t>(ColorWriteMask::kAll);
512+
ColorWriteMask write_mask = ColorWriteMaskBits::kAll;
513513

514514
constexpr bool operator==(const ColorAttachmentDescriptor& o) const {
515515
return format == o.format && //
@@ -524,10 +524,10 @@ struct ColorAttachmentDescriptor {
524524
}
525525

526526
constexpr size_t Hash() const {
527-
return fml::HashCombine(format, blending_enabled, src_color_blend_factor,
528-
color_blend_op, dst_color_blend_factor,
529-
src_alpha_blend_factor, alpha_blend_op,
530-
dst_alpha_blend_factor, write_mask);
527+
return fml::HashCombine(
528+
format, blending_enabled, src_color_blend_factor, color_blend_op,
529+
dst_color_blend_factor, src_alpha_blend_factor, alpha_blend_op,
530+
dst_alpha_blend_factor, static_cast<uint64_t>(write_mask));
531531
}
532532
};
533533

‎impeller/core/texture_descriptor.h‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -40,8 +40,7 @@ struct TextureDescriptor {
4040
PixelFormat format = PixelFormat::kUnknown;
4141
ISize size;
4242
size_t mip_count = 1u; // Size::MipCount is usually appropriate.
43-
TextureUsageMask usage =
44-
static_cast<TextureUsageMask>(TextureUsage::kShaderRead);
43+
TextureUsageMask usage = TextureUsage::kShaderRead;
4544
SampleCount sample_count = SampleCount::kCount1;
4645
CompressionType compression_type = CompressionType::kLossless;
4746

‎impeller/entity/contents/content_context.cc‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -458,8 +458,7 @@ ContentContext::ContentContext(
458458
auto clip_color_attachments =
459459
clip_pipeline_descriptor->GetColorAttachmentDescriptors();
460460
for (auto& color_attachment : clip_color_attachments) {
461-
color_attachment.second.write_mask =
462-
static_cast<uint64_t>(ColorWriteMask::kNone);
461+
color_attachment.second.write_mask = ColorWriteMaskBits::kNone;
463462
}
464463
clip_pipeline_descriptor->SetColorAttachmentDescriptors(
465464
std::move(clip_color_attachments));

‎impeller/entity/entity_pass_target_unittests.cc‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -71,7 +71,7 @@ TEST_P(EntityPassTargetTest, SwapWithMSAAImplicitResolve) {
7171
color0_tex_desc.sample_count = SampleCount::kCount4;
7272
color0_tex_desc.format = pixel_format;
7373
color0_tex_desc.size = ISize{100, 100};
74-
color0_tex_desc.usage = static_cast<uint64_t>(TextureUsage::kRenderTarget);
74+
color0_tex_desc.usage = TextureUsage::kRenderTarget;
7575

7676
auto color0_msaa_tex = allocator.CreateTexture(color0_tex_desc);
7777

‎impeller/renderer/backend/gles/render_pass_gles.cc‎

Lines changed: 7 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -59,18 +59,16 @@ void ConfigureBlending(const ProcTableGLES& gl,
5959
}
6060

6161
{
62-
const auto is_set = [](std::underlying_type_t<ColorWriteMask> mask,
62+
const auto is_set = [](ColorWriteMask mask,
6363
ColorWriteMask check) -> GLboolean {
64-
using RawType = decltype(mask);
65-
return (static_cast<RawType>(mask) & static_cast<RawType>(check))
66-
? GL_TRUE
67-
: GL_FALSE;
64+
return (mask & check) ? GL_TRUE : GL_FALSE;
6865
};
6966

70-
gl.ColorMask(is_set(color->write_mask, ColorWriteMask::kRed), // red
71-
is_set(color->write_mask, ColorWriteMask::kGreen), // green
72-
is_set(color->write_mask, ColorWriteMask::kBlue), // blue
73-
is_set(color->write_mask, ColorWriteMask::kAlpha) // alpha
67+
gl.ColorMask(
68+
is_set(color->write_mask, ColorWriteMaskBits::kRed), // red
69+
is_set(color->write_mask, ColorWriteMaskBits::kGreen), // green
70+
is_set(color->write_mask, ColorWriteMaskBits::kBlue), // blue
71+
is_set(color->write_mask, ColorWriteMaskBits::kAlpha) // alpha
7472
);
7573
}
7674
}

‎impeller/renderer/backend/gles/surface_gles.cc‎

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,7 @@ std::unique_ptr<Surface> SurfaceGLES::WrapFBO(
2929
color0_tex.type = TextureType::kTexture2D;
3030
color0_tex.format = color_format;
3131
color0_tex.size = fbo_size;
32-
color0_tex.usage = static_cast<TextureUsageMask>(TextureUsage::kRenderTarget);
32+
color0_tex.usage = TextureUsage::kRenderTarget;
3333
color0_tex.sample_count = SampleCount::kCount1;
3434
color0_tex.storage_mode = StorageMode::kDevicePrivate;
3535

@@ -44,8 +44,7 @@ std::unique_ptr<Surface> SurfaceGLES::WrapFBO(
4444
depth_stencil_texture_desc.type = TextureType::kTexture2D;
4545
depth_stencil_texture_desc.format = color_format;
4646
depth_stencil_texture_desc.size = fbo_size;
47-
depth_stencil_texture_desc.usage =
48-
static_cast<TextureUsageMask>(TextureUsage::kRenderTarget);
47+
depth_stencil_texture_desc.usage = TextureUsage::kRenderTarget;
4948
depth_stencil_texture_desc.sample_count = SampleCount::kCount1;
5049

5150
auto depth_stencil_tex = std::make_shared<TextureGLES>(

‎impeller/renderer/backend/gles/texture_gles.cc‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -45,8 +45,7 @@ static bool IsDepthStencilFormat(PixelFormat format) {
4545
static TextureGLES::Type GetTextureTypeFromDescriptor(
4646
const TextureDescriptor& desc) {
4747
const auto usage = static_cast<TextureUsageMask>(desc.usage);
48-
const auto render_target =
49-
static_cast<TextureUsageMask>(TextureUsage::kRenderTarget);
48+
const auto render_target = TextureUsage::kRenderTarget;
5049
const auto is_msaa = desc.sample_count == SampleCount::kCount4;
5150
if (usage == render_target && IsDepthStencilFormat(desc.format)) {
5251
return is_msaa ? TextureGLES::Type::kRenderBufferMultisampled

0 commit comments

Comments
 (0)