Skip to content

Commit

Permalink
Clang tidy-ify DlColor and friends. (flutter#46122)
Browse files Browse the repository at this point in the history 
Closes flutter/flutter#135057.

This is a fair bit more involved than previous changes, just due to the sheer number of implicit conversions.

Highlights:

- Made `public uint32_t argb` `private uint32_t argb_`, and added `argb()` instead.
- Added `ToSk(DlColor)` instead of using implicit conversions.

There were a bunch of places where I had to make a judgement call (particularly in tests) to keep the code a bit "messy", i.e. `DlColor(SK_RED)`, just to make the diff as small as possible and to prevent silly copy and paste bugs. I'd be open to filing a follow-up issue to reduce unnecessary wrapping.
  • Loading branch information
@matanlurey
matanlurey authored Sep 20, 2023
1 parent 84f1d3b commit a15b93d
Show file tree
Hide file tree
Showing 36 changed files with 196 additions and 163 deletions.
21 changes: 11 additions & 10 deletions display_list/benchmarking/dl_benchmarks.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -710,15 +710,15 @@ std::shared_ptr<DlVertices> GetTestVertices(SkPoint center,
// the center point C, this should create a triangle fan with vertices
// C, O_0, O_1, O_2, O_3, ...
vertices.push_back(center);
colors.push_back(SK_ColorCYAN);
colors.push_back(DlColor(SK_ColorCYAN));
for (size_t i = 0; i <= outer_points.size(); i++) {
vertices.push_back(outer_points[i % outer_points.size()]);
if (i % 3 == 0) {
colors.push_back(SK_ColorRED);
colors.push_back(DlColor(SK_ColorRED));
} else if (i % 3 == 1) {
colors.push_back(SK_ColorGREEN);
colors.push_back(DlColor(SK_ColorGREEN));
} else {
colors.push_back(SK_ColorBLUE);
colors.push_back(DlColor(SK_ColorBLUE));
}
}
break;
Expand All @@ -728,11 +728,11 @@ std::shared_ptr<DlVertices> GetTestVertices(SkPoint center,
// vertices O_0, O_1, C, O_1, O_2, C, O_2, O_3, C, ...
for (size_t i = 0; i < outer_vertex_count; i++) {
vertices.push_back(outer_points[i % outer_points.size()]);
colors.push_back(SK_ColorRED);
colors.push_back(DlColor(SK_ColorRED));
vertices.push_back(outer_points[(i + 1) % outer_points.size()]);
colors.push_back(SK_ColorGREEN);
colors.push_back(DlColor(SK_ColorGREEN));
vertices.push_back(center);
colors.push_back(SK_ColorBLUE);
colors.push_back(DlColor(SK_ColorBLUE));
}
break;
case DlVertexMode::kTriangleStrip:
Expand All @@ -741,10 +741,10 @@ std::shared_ptr<DlVertices> GetTestVertices(SkPoint center,
// O_0, O_1, C, O_2, O_3, C, O_4, O_5, C, ...
for (size_t i = 0; i <= outer_vertex_count; i++) {
vertices.push_back(outer_points[i % outer_points.size()]);
colors.push_back(i % 2 ? SK_ColorRED : SK_ColorGREEN);
colors.push_back(i % 2 ? DlColor(SK_ColorRED) : DlColor(SK_ColorGREEN));
if (i % 2 == 1) {
vertices.push_back(center);
colors.push_back(SK_ColorBLUE);
colors.push_back(DlColor(SK_ColorBLUE));
}
}
break;
Expand Down Expand Up @@ -1270,7 +1270,8 @@ void BM_DrawShadow(benchmark::State& state,

// We can hardcode dpr to 1.0f as we're varying elevation, and dpr is only
// ever used in conjunction with elevation.
builder.DrawShadow(path, SK_ColorBLUE, elevation, transparent_occluder, 1.0f);
builder.DrawShadow(path, DlColor(SK_ColorBLUE), elevation,
transparent_occluder, 1.0f);
auto display_list = builder.Build();

// We only want to time the actual rasterization.
Expand Down
10 changes: 6 additions & 4 deletions display_list/benchmarking/dl_complexity_unittests.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -190,31 +190,33 @@ TEST(DisplayListComplexity, DrawShadow) {
line_path.moveTo(SkPoint::Make(0, 0));
line_path.lineTo(SkPoint::Make(10, 10));
line_path.close();
builder_line.DrawShadow(line_path, SK_ColorRED, 10.0f, false, 1.0f);
builder_line.DrawShadow(line_path, DlColor(SK_ColorRED), 10.0f, false, 1.0f);
auto display_list_line = builder_line.Build();

DisplayListBuilder builder_quad;
SkPath quad_path;
quad_path.moveTo(SkPoint::Make(0, 0));
quad_path.quadTo(SkPoint::Make(10, 10), SkPoint::Make(10, 20));
quad_path.close();
builder_quad.DrawShadow(quad_path, SK_ColorRED, 10.0f, false, 1.0f);
builder_quad.DrawShadow(quad_path, DlColor(SK_ColorRED), 10.0f, false, 1.0f);
auto display_list_quad = builder_quad.Build();

DisplayListBuilder builder_conic;
SkPath conic_path;
conic_path.moveTo(SkPoint::Make(0, 0));
conic_path.conicTo(SkPoint::Make(10, 10), SkPoint::Make(10, 20), 1.5f);
conic_path.close();
builder_conic.DrawShadow(conic_path, SK_ColorRED, 10.0f, false, 1.0f);
builder_conic.DrawShadow(conic_path, DlColor(SK_ColorRED), 10.0f, false,
1.0f);
auto display_list_conic = builder_conic.Build();

DisplayListBuilder builder_cubic;
SkPath cubic_path;
cubic_path.moveTo(SkPoint::Make(0, 0));
cubic_path.cubicTo(SkPoint::Make(10, 10), SkPoint::Make(10, 20),
SkPoint::Make(20, 20));
builder_cubic.DrawShadow(cubic_path, SK_ColorRED, 10.0f, false, 1.0f);
builder_cubic.DrawShadow(cubic_path, DlColor(SK_ColorRED), 10.0f, false,
1.0f);
auto display_list_cubic = builder_cubic.Build();

auto calculators = AccumulatorCalculators();
Expand Down
34 changes: 19 additions & 15 deletions display_list/display_list_unittests.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -932,7 +932,8 @@ TEST_F(DisplayListTest, NestedOpCountMetricsSameAsSkPicture) {
DlOpReceiver& receiver = ToReceiver(builder);
for (int y = 10; y <= 60; y += 10) {
for (int x = 10; x <= 60; x += 10) {
receiver.setColor(((x + y) % 20) == 10 ? SK_ColorRED : SK_ColorBLUE);
receiver.setColor(((x + y) % 20) == 10 ? DlColor(SK_ColorRED)
: DlColor(SK_ColorBLUE));
receiver.drawRect(SkRect::MakeXYWH(x, y, 80, 80));
}
}
Expand Down Expand Up @@ -1066,8 +1067,10 @@ TEST_F(DisplayListTest, SingleOpsMightSupportGroupOpacityBlendMode) {
#body, [](DlOpReceiver& receiver) { body }, expect, expect)

RUN_TESTS(receiver.drawPaint(););
RUN_TESTS2(receiver.drawColor(SK_ColorRED, DlBlendMode::kSrcOver);, true);
RUN_TESTS2(receiver.drawColor(SK_ColorRED, DlBlendMode::kSrc);, false);
RUN_TESTS2(receiver.drawColor(DlColor(SK_ColorRED), DlBlendMode::kSrcOver);
, true);
RUN_TESTS2(receiver.drawColor(DlColor(SK_ColorRED), DlBlendMode::kSrc);
, false);
RUN_TESTS(receiver.drawLine({0, 0}, {10, 10}););
RUN_TESTS(receiver.drawRect({0, 0, 10, 10}););
RUN_TESTS(receiver.drawOval({0, 0, 10, 10}););
Expand Down Expand Up @@ -1119,8 +1122,9 @@ TEST_F(DisplayListTest, SingleOpsMightSupportGroupOpacityBlendMode) {
RUN_TESTS2(receiver.drawDisplayList(display_list);, false);
}
RUN_TESTS2(receiver.drawTextBlob(TestBlob1, 0, 0);, false);
RUN_TESTS2(receiver.drawShadow(kTestPath1, SK_ColorBLACK, 1.0, false, 1.0);
, false);
RUN_TESTS2(
receiver.drawShadow(kTestPath1, DlColor(SK_ColorBLACK), 1.0, false, 1.0);
, false);

#undef RUN_TESTS2
#undef RUN_TESTS
Expand Down Expand Up @@ -1250,7 +1254,7 @@ TEST_F(DisplayListTest, SaveLayerOneSimpleOpInheritsOpacity) {

DisplayListBuilder builder;
DlOpReceiver& receiver = ToReceiver(builder);
receiver.setColor(SkColorSetARGB(127, 255, 255, 255));
receiver.setColor(DlColor(SkColorSetARGB(127, 255, 255, 255)));
receiver.saveLayer(nullptr, SaveLayerOptions::kWithAttributes);
receiver.drawRect({10, 10, 20, 20});
receiver.restore();
Expand Down Expand Up @@ -1280,7 +1284,7 @@ TEST_F(DisplayListTest, SaveLayerTwoOverlappingOpsDoesNotInheritOpacity) {

DisplayListBuilder builder;
DlOpReceiver& receiver = ToReceiver(builder);
receiver.setColor(SkColorSetARGB(127, 255, 255, 255));
receiver.setColor(DlColor(SkColorSetARGB(127, 255, 255, 255)));
receiver.saveLayer(nullptr, SaveLayerOptions::kWithAttributes);
receiver.drawRect({10, 10, 20, 20});
receiver.drawRect({15, 15, 25, 25});
Expand All @@ -1300,7 +1304,7 @@ TEST_F(DisplayListTest, NestedSaveLayersMightInheritOpacity) {

DisplayListBuilder builder;
DlOpReceiver& receiver = ToReceiver(builder);
receiver.setColor(SkColorSetARGB(127, 255, 255, 255));
receiver.setColor(DlColor(SkColorSetARGB(127, 255, 255, 255)));
receiver.saveLayer(nullptr, SaveLayerOptions::kWithAttributes);
receiver.saveLayer(nullptr, SaveLayerOptions::kWithAttributes);
receiver.drawRect({10, 10, 20, 20});
Expand All @@ -1323,7 +1327,7 @@ TEST_F(DisplayListTest, NestedSaveLayersCanBothSupportOpacityOptimization) {

DisplayListBuilder builder;
DlOpReceiver& receiver = ToReceiver(builder);
receiver.setColor(SkColorSetARGB(127, 255, 255, 255));
receiver.setColor(DlColor(SkColorSetARGB(127, 255, 255, 255)));
receiver.saveLayer(nullptr, SaveLayerOptions::kWithAttributes);
receiver.saveLayer(nullptr, SaveLayerOptions::kNoAttributes);
receiver.drawRect({10, 10, 20, 20});
Expand All @@ -1340,7 +1344,7 @@ TEST_F(DisplayListTest, SaveLayerImageFilterDoesNotInheritOpacity) {

DisplayListBuilder builder;
DlOpReceiver& receiver = ToReceiver(builder);
receiver.setColor(SkColorSetARGB(127, 255, 255, 255));
receiver.setColor(DlColor(SkColorSetARGB(127, 255, 255, 255)));
receiver.setImageFilter(&kTestBlurImageFilter1);
receiver.saveLayer(nullptr, SaveLayerOptions::kWithAttributes);
receiver.setImageFilter(nullptr);
Expand All @@ -1357,7 +1361,7 @@ TEST_F(DisplayListTest, SaveLayerColorFilterDoesNotInheritOpacity) {

DisplayListBuilder builder;
DlOpReceiver& receiver = ToReceiver(builder);
receiver.setColor(SkColorSetARGB(127, 255, 255, 255));
receiver.setColor(DlColor(SkColorSetARGB(127, 255, 255, 255)));
receiver.setColorFilter(&kTestMatrixColorFilter1);
receiver.saveLayer(nullptr, SaveLayerOptions::kWithAttributes);
receiver.setColorFilter(nullptr);
Expand All @@ -1374,7 +1378,7 @@ TEST_F(DisplayListTest, SaveLayerSrcBlendDoesNotInheritOpacity) {

DisplayListBuilder builder;
DlOpReceiver& receiver = ToReceiver(builder);
receiver.setColor(SkColorSetARGB(127, 255, 255, 255));
receiver.setColor(DlColor(SkColorSetARGB(127, 255, 255, 255)));
receiver.setBlendMode(DlBlendMode::kSrc);
receiver.saveLayer(nullptr, SaveLayerOptions::kWithAttributes);
receiver.setBlendMode(DlBlendMode::kSrcOver);
Expand All @@ -1392,7 +1396,7 @@ TEST_F(DisplayListTest, SaveLayerImageFilterOnChildInheritsOpacity) {

DisplayListBuilder builder;
DlOpReceiver& receiver = ToReceiver(builder);
receiver.setColor(SkColorSetARGB(127, 255, 255, 255));
receiver.setColor(DlColor(SkColorSetARGB(127, 255, 255, 255)));
receiver.saveLayer(nullptr, SaveLayerOptions::kWithAttributes);
receiver.setImageFilter(&kTestBlurImageFilter1);
receiver.drawRect({10, 10, 20, 20});
Expand All @@ -1408,7 +1412,7 @@ TEST_F(DisplayListTest, SaveLayerColorFilterOnChildDoesNotInheritOpacity) {

DisplayListBuilder builder;
DlOpReceiver& receiver = ToReceiver(builder);
receiver.setColor(SkColorSetARGB(127, 255, 255, 255));
receiver.setColor(DlColor(SkColorSetARGB(127, 255, 255, 255)));
receiver.saveLayer(nullptr, SaveLayerOptions::kWithAttributes);
receiver.setColorFilter(&kTestMatrixColorFilter1);
receiver.drawRect({10, 10, 20, 20});
Expand All @@ -1424,7 +1428,7 @@ TEST_F(DisplayListTest, SaveLayerSrcBlendOnChildDoesNotInheritOpacity) {

DisplayListBuilder builder;
DlOpReceiver& receiver = ToReceiver(builder);
receiver.setColor(SkColorSetARGB(127, 255, 255, 255));
receiver.setColor(DlColor(SkColorSetARGB(127, 255, 255, 255)));
receiver.saveLayer(nullptr, SaveLayerOptions::kWithAttributes);
receiver.setBlendMode(DlBlendMode::kSrc);
receiver.drawRect({10, 10, 20, 20});
Expand Down
2 changes: 1 addition & 1 deletion display_list/dl_builder.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -351,7 +351,7 @@ void DisplayListBuilder::SetAttributesFromPaint(
setDither(paint.isDither());
}
if (flags.applies_alpha_or_color()) {
setColor(paint.getColor().argb);
setColor(paint.getColor());
}
if (flags.applies_blend()) {
setBlendMode(paint.getBlendMode());
Expand Down
63 changes: 32 additions & 31 deletions display_list/dl_color.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,36 +11,34 @@ namespace flutter {

struct DlColor {
public:
constexpr DlColor() : argb(0xFF000000) {}
constexpr DlColor(uint32_t argb) : argb(argb) {}
constexpr DlColor() : argb_(0xFF000000) {}
constexpr explicit DlColor(uint32_t argb) : argb_(argb) {}

static constexpr uint8_t toAlpha(SkScalar opacity) { return toC(opacity); }
static constexpr SkScalar toOpacity(uint8_t alpha) { return toF(alpha); }

// clang-format off
static constexpr DlColor kTransparent() {return 0x00000000;};
static constexpr DlColor kBlack() {return 0xFF000000;};
static constexpr DlColor kWhite() {return 0xFFFFFFFF;};
static constexpr DlColor kRed() {return 0xFFFF0000;};
static constexpr DlColor kGreen() {return 0xFF00FF00;};
static constexpr DlColor kBlue() {return 0xFF0000FF;};
static constexpr DlColor kCyan() {return 0xFF00FFFF;};
static constexpr DlColor kMagenta() {return 0xFFFF00FF;};
static constexpr DlColor kYellow() {return 0xFFFFFF00;};
static constexpr DlColor kDarkGrey() {return 0xFF3F3F3F;};
static constexpr DlColor kMidGrey() {return 0xFF808080;};
static constexpr DlColor kLightGrey() {return 0xFFC0C0C0;};
static constexpr DlColor kTransparent() {return DlColor(0x00000000);};
static constexpr DlColor kBlack() {return DlColor(0xFF000000);};
static constexpr DlColor kWhite() {return DlColor(0xFFFFFFFF);};
static constexpr DlColor kRed() {return DlColor(0xFFFF0000);};
static constexpr DlColor kGreen() {return DlColor(0xFF00FF00);};
static constexpr DlColor kBlue() {return DlColor(0xFF0000FF);};
static constexpr DlColor kCyan() {return DlColor(0xFF00FFFF);};
static constexpr DlColor kMagenta() {return DlColor(0xFFFF00FF);};
static constexpr DlColor kYellow() {return DlColor(0xFFFFFF00);};
static constexpr DlColor kDarkGrey() {return DlColor(0xFF3F3F3F);};
static constexpr DlColor kMidGrey() {return DlColor(0xFF808080);};
static constexpr DlColor kLightGrey() {return DlColor(0xFFC0C0C0);};
// clang-format on

uint32_t argb;

constexpr bool isOpaque() const { return getAlpha() == 0xFF; }
constexpr bool isTransparent() const { return getAlpha() == 0; }

constexpr int getAlpha() const { return argb >> 24; }
constexpr int getRed() const { return (argb >> 16) & 0xFF; }
constexpr int getGreen() const { return (argb >> 8) & 0xFF; }
constexpr int getBlue() const { return argb & 0xFF; }
constexpr int getAlpha() const { return argb_ >> 24; }
constexpr int getRed() const { return (argb_ >> 16) & 0xFF; }
constexpr int getGreen() const { return (argb_ >> 8) & 0xFF; }
constexpr int getBlue() const { return argb_ & 0xFF; }

constexpr float getAlphaF() const { return toF(getAlpha()); }
constexpr float getRedF() const { return toF(getRed()); }
Expand All @@ -49,26 +47,26 @@ struct DlColor {

constexpr uint32_t premultipliedArgb() const {
if (isOpaque()) {
return argb;
return argb_;
}
float f = getAlphaF();
return (argb & 0xFF000000) | //
return (argb_ & 0xFF000000) | //
toC(getRedF() * f) << 16 | //
toC(getGreenF() * f) << 8 | //
toC(getBlueF() * f);
}

constexpr DlColor withAlpha(uint8_t alpha) const { //
return (argb & 0x00FFFFFF) | (alpha << 24);
return DlColor((argb_ & 0x00FFFFFF) | (alpha << 24));
}
constexpr DlColor withRed(uint8_t red) const { //
return (argb & 0xFF00FFFF) | (red << 16);
return DlColor((argb_ & 0xFF00FFFF) | (red << 16));
}
constexpr DlColor withGreen(uint8_t green) const { //
return (argb & 0xFFFF00FF) | (green << 8);
return DlColor((argb_ & 0xFFFF00FF) | (green << 8));
}
constexpr DlColor withBlue(uint8_t blue) const { //
return (argb & 0xFFFFFF00) | (blue << 0);
return DlColor((argb_ & 0xFFFFFF00) | (blue << 0));
}

constexpr DlColor modulateOpacity(float opacity) const {
Expand All @@ -77,13 +75,16 @@ struct DlColor {
: withAlpha(round(getAlpha() * opacity));
}

operator uint32_t() const { return argb; }
bool operator==(DlColor const& other) const { return argb == other.argb; }
bool operator!=(DlColor const& other) const { return argb != other.argb; }
bool operator==(uint32_t const& other) const { return argb == other; }
bool operator!=(uint32_t const& other) const { return argb != other; }
constexpr uint32_t argb() const { return argb_; }

bool operator==(DlColor const& other) const { return argb_ == other.argb_; }
bool operator!=(DlColor const& other) const { return argb_ != other.argb_; }
bool operator==(uint32_t const& other) const { return argb_ == other; }
bool operator!=(uint32_t const& other) const { return argb_ != other; }

private:
uint32_t argb_;

static float toF(uint8_t comp) { return comp * (1.0f / 255); }
static uint8_t toC(float fComp) { return round(fComp * 255); }
};
Expand Down
2 changes: 1 addition & 1 deletion display_list/dl_color_unittests.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ static void arraysEqual(const uint32_t* ints,
const DlColor* colors,
int count) {
for (int i = 0; i < count; i++) {
EXPECT_TRUE(ints[i] == colors[i]);
EXPECT_TRUE(ints[i] == colors[i].argb());
}
}

Expand Down
7 changes: 2 additions & 5 deletions display_list/dl_paint.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -78,11 +78,8 @@ class DlPaint {
return *this;
}

uint8_t getAlpha() const { return color_.argb >> 24; }
DlPaint& setAlpha(uint8_t alpha) {
color_.argb = alpha << 24 | (color_.argb & 0x00FFFFFF);
return *this;
}
uint8_t getAlpha() const { return color_.argb() >> 24; }
DlPaint& setAlpha(uint8_t alpha) { return setColor(color_.withAlpha(alpha)); }
SkScalar getOpacity() const { return color_.getAlphaF(); }
DlPaint& setOpacity(SkScalar opacity) {
setAlpha(SkScalarRoundToInt(opacity * 0xff));
Expand Down
4 changes: 2 additions & 2 deletions display_list/dl_paint_unittests.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -42,14 +42,14 @@ TEST(DisplayListPaint, ConstructorDefaults) {

EXPECT_EQ(paint, DlPaint());
EXPECT_EQ(paint, DlPaint(DlColor::kBlack()));
EXPECT_EQ(paint, DlPaint(0xFF000000));
EXPECT_EQ(paint, DlPaint(DlColor(0xFF000000)));

EXPECT_NE(paint, DlPaint().setAntiAlias(true));
EXPECT_NE(paint, DlPaint().setDither(true));
EXPECT_NE(paint, DlPaint().setInvertColors(true));
EXPECT_NE(paint, DlPaint().setColor(DlColor::kGreen()));
EXPECT_NE(paint, DlPaint(DlColor::kGreen()));
EXPECT_NE(paint, DlPaint(0xFF00FF00));
EXPECT_NE(paint, DlPaint(DlColor(0xFF00FF00)));
EXPECT_NE(paint, DlPaint().setAlpha(0x7f));
EXPECT_NE(paint, DlPaint().setBlendMode(DlBlendMode::kDstIn));
EXPECT_NE(paint, DlPaint().setDrawStyle(DlDrawStyle::kStrokeAndFill));
Expand Down
4 changes: 2 additions & 2 deletions display_list/effects/dl_color_source.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -191,7 +191,7 @@ class DlColorColorSource final : public DlColorSource {
DlColorSourceType type() const override { return DlColorSourceType::kColor; }
size_t size() const override { return sizeof(*this); }

bool is_opaque() const override { return (color_ >> 24) == 255; }
bool is_opaque() const override { return color_.getAlpha() == 255; }

DlColor color() const { return color_; }

Expand Down Expand Up @@ -290,7 +290,7 @@ class DlGradientColorSourceBase : public DlMatrixColorSourceBase {
}
const DlColor* my_colors = colors();
for (uint32_t i = 0; i < stop_count_; i++) {
if ((my_colors[i] >> 24) < 255) {
if (my_colors[i].getAlpha() < 255) {
return false;
}
}
Expand Down
Loading

0 comments on commit a15b93d

Please sign in to comment.