Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1506253002:
Remove no-op code in SkAlphaRuns::add (Closed)
DescriptionRemove no-op code in SkAlphaRuns::add
The old code says
runs += x + 1;
alpha += x + 1;
x = 0;
lastAlpha += x; // we don't want the +1
The last line does nothing. Remove this unnecessary line.
BUG=skia:401
Committed: https://skia.googlesource.com/skia/+/dc877a7734151a53858d9f5a777d1aeb32290ffc
Patch Set 1 #Patch Set 2 : Delete the meaningless line instead #Messages
Total messages: 24 (8 generated)
scroggo@google.com changed reviewers: + reed@google.com
how (many) do the GM images change?
On 2015/12/09 15:58:05, reed1 wrote: > how (many) do the GM images change? I only ran a single (arbitrary) trybot, and https://gold.skia.org/trybot lists my CL, but clicking the link shows: No digests match your query. Displaying 0 of 0 Digests that matched your query. I don't know whether that means nothing changed, Gold threw them out (how long will it hang on to my images?), none of our GMs test it, or it did not make a difference in drawing. It appears this method is only called in one place: in SuperBlitter::blitH, which is only used for anti aliased clips/paths. I assume that is covered by our GMs. Looking at that method in more detail, lastAlpha may get modified after this, without being read. The only time this setting of lastAlpha is meaningful is when (startAlpha && !middleCount && !stopAlpha). I don't know how common that is, but if it is rare (or never) that would explain why we don't see any differences.
The CQ bit was checked by mtklein@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1506253002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1506253002/1
On 2015/12/09 at 16:27:37, scroggo wrote: > On 2015/12/09 15:58:05, reed1 wrote: > > how (many) do the GM images change? > > I only ran a single (arbitrary) trybot, and https://gold.skia.org/trybot lists my CL, but clicking the link shows: > > No digests match your query. > Displaying 0 of 0 Digests that matched your query. > > I don't know whether that means nothing changed, Gold threw them out (how long will it hang on to my images?), none of our GMs test it, or it did not make a difference in drawing. Could be any but "Gold threw them out". They'll be there for a long time.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The link to see GM diffs is: https://gold.skia.org/search2?issue=1506253002&unt=true&query=source_type%3Dg...
On 2015/12/09 16:48:57, mtklein wrote: > The link to see GM diffs is: > > https://gold.skia.org/search2?issue=1506253002&unt=true&query=source_type%3Dg... Thanks! Yeah, it did not change any of our GMs.
Given that nothing changed (afawk), and we don't understand the change enough to want it anyway (is that true?), I vote to just delete the dead code.
The CQ bit was checked by scroggo@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1506253002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1506253002/20001
Description was changed from
==========
Follow the intent of comment in SkAlphaRuns::add
The old code says
runs += x + 1;
alpha += x + 1;
x = 0;
lastAlpha += x; // we don't want the +1
The last line does nothing, but the comment seems to indicate that we
want to add x before we set it to zero. Follow the intent of the
comment.
BUG=skia:401
==========
to
==========
Remove no-op code in SkAlphaRuns::add
The old code says
runs += x + 1;
alpha += x + 1;
x = 0;
lastAlpha += x; // we don't want the +1
The last line does nothing. Remove this unnecessary line.
BUG=skia:401
==========
On 2015/12/09 17:10:17, reed1 wrote: > Given that nothing changed (afawk), We could write some GMs that draw/clip paths with antialiasing. Presumably we already cover that (I haven't looked though). > and we don't understand the change enough to > want it anyway (is that true?), Looking at the git blame, I think you, mtklein@, and tomhudson@ are the last ones to touch this code. > I vote to just delete the dead code. sgtm. Unless this code is responsible for some bug that we don't understand, we appear to be fine with the current behavior.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2015/12/09 19:36:55, commit-bot: I haz the power wrote: > Dry run: This issue passed the CQ dry run. PTAL at patch set 2, which removes the no-op code.
lgtm
The CQ bit was checked by scroggo@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1506253002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1506253002/20001
Message was sent while issue was closed.
Description was changed from
==========
Remove no-op code in SkAlphaRuns::add
The old code says
runs += x + 1;
alpha += x + 1;
x = 0;
lastAlpha += x; // we don't want the +1
The last line does nothing. Remove this unnecessary line.
BUG=skia:401
==========
to
==========
Remove no-op code in SkAlphaRuns::add
The old code says
runs += x + 1;
alpha += x + 1;
x = 0;
lastAlpha += x; // we don't want the +1
The last line does nothing. Remove this unnecessary line.
BUG=skia:401
Committed:
https://skia.googlesource.com/skia/+/dc877a7734151a53858d9f5a777d1aeb32290ffc
==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://skia.googlesource.com/skia/+/dc877a7734151a53858d9f5a777d1aeb32290ffc |
