nsSprocketLayout::PopulateBoxSizes: Value stored to 'ascent' is never read

RESOLVED FIXED in Firefox 57

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: sylvestre, Assigned: sajattack, Mentored)

Tracking

(Blocks: 1 bug, {good-first-bug})

Trunk
mozilla57
good-first-bug
Points:
---

Firefox Tracking Flags

(firefox57 fixed)

Details

(Whiteboard: [good first bug][lang=C++])

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Comment 1

2 years ago
Should I remove the line where ascent is declared as well?
(Assignee)

Comment 3

2 years ago
MozReview-Commit-ID: 3VXaVgBhgTL
(Assignee)

Comment 4

2 years ago
Posted patch my-change.patch (obsolete) — Splinter Review
Attachment #8906484 - Flags: review+

Comment 5

2 years ago
mozreview-review
Comment on attachment 8906482 [details]
Bug 1398623 - Remove several unused variables in nsSprocketLayout::PopulateBoxSizes

https://reviewboard.mozilla.org/r/178242/#review183146

::: layout/xul/nsSprocketLayout.cpp
(Diff revision 1)
>        minSize = child->GetXULMinSize(aState);
>        maxSize = nsBox::BoundsCheckMinMax(minSize, child->GetXULMaxSize(aState));
>        ascent = child->GetXULBoxAscent(aState);
>        nsMargin margin;
>        child->GetXULMargin(margin);
> -      ascent += margin.top;

If `ascent` is never read, shouldn't you be able to remove the assignment statement above as well as the declaration of `ascent` itself?
(Assignee)

Comment 6

2 years ago
I was wondering that as well.
I think that can be removed as well. And it seems with `ascent` removed, `margin` can also be removed.
(Assignee)

Comment 8

2 years ago
mozreview-review-reply
Comment on attachment 8906482 [details]
Bug 1398623 - Remove several unused variables in nsSprocketLayout::PopulateBoxSizes

https://reviewboard.mozilla.org/r/178242/#review183146

> If `ascent` is never read, shouldn't you be able to remove the assignment statement above as well as the declaration of `ascent` itself?

I was wondering that as well.
Assignee: nobody → sajattack
Comment hidden (mozreview-request)

Comment 10

2 years ago
mozreview-review
Comment on attachment 8906492 [details]
Bug 1398623 - Remove additional declarations

https://reviewboard.mozilla.org/r/178256/#review183152

Please squash your two commits together. They are doing the same thing.

::: layout/xul/nsSprocketLayout.cpp
(Diff revision 1)
> -      ascent = child->GetXULBoxAscent(aState);
> -      nsMargin margin;
>        child->GetXULMargin(margin);

You need to remove the `child->GetXULMargin(margin);` line as well.

And you can remove the `nscoord ascent = 0;` line above. (Actually you would have to remove that line, otherwise some compilers may complains on unused variable.)
Attachment #8906492 - Flags: review?(xidorn+moz)
Attachment #8906484 - Attachment is obsolete: true
Attachment #8906483 - Attachment is obsolete: true
(Assignee)

Comment 11

2 years ago
How do I squash the commits?
If you use mercurial, you can use
> hg histedit
and mark the second commit as "roll" to squash the two commits.

If you use git, I think "git rebase -i" would work in a similar way, but I'm not familiar with git development on mozilla repo.
(Assignee)

Comment 13

2 years ago
mozreview-review-reply
Comment on attachment 8906492 [details]
Bug 1398623 - Remove additional declarations

https://reviewboard.mozilla.org/r/178256/#review183152

> You need to remove the `child->GetXULMargin(margin);` line as well.
> 
> And you can remove the `nscoord ascent = 0;` line above. (Actually you would have to remove that line, otherwise some compilers may complains on unused variable.)

Should I remove the whole while loop from lines 1507 to 1530? It's using ascent and margin.

Comment 14

2 years ago
mozreview-review-reply
Comment on attachment 8906492 [details]
Bug 1398623 - Remove additional declarations

https://reviewboard.mozilla.org/r/178256/#review183152

> Should I remove the whole while loop from lines 1507 to 1530? It's using ascent and margin.

I'm not sure why are you thinking about that. It doesn't seem to me that the code you mentioned is related to the unused variables we are removing here.

I mentioned that we can remove `child->GetXULMargin(margin);` because that method call is for getting the value and storing the result into `margin`. Given that we no longer need the `margin` variable here, filling value into this variable becomes useles as well.
(Assignee)

Comment 15

2 years ago
mozreview-review-reply
Comment on attachment 8906492 [details]
Bug 1398623 - Remove additional declarations

https://reviewboard.mozilla.org/r/178256/#review183152

> I'm not sure why are you thinking about that. It doesn't seem to me that the code you mentioned is related to the unused variables we are removing here.
> 
> I mentioned that we can remove `child->GetXULMargin(margin);` because that method call is for getting the value and storing the result into `margin`. Given that we no longer need the `margin` variable here, filling value into this variable becomes useles as well.

I did a search for "nscoord ascent" and it came up in that loop.
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Attachment #8906492 - Attachment is obsolete: true

Comment 17

2 years ago
mozreview-review
Comment on attachment 8906482 [details]
Bug 1398623 - Remove several unused variables in nsSprocketLayout::PopulateBoxSizes

https://reviewboard.mozilla.org/r/178242/#review183164

r=me with the commit message updated.

::: commit-message-a5f16:1
(Diff revision 2)
> +Bug 1398623 - remove "ascent += margin.top;"

Now this commit is not just removing the single statement. Consider change the commit message to something like
> Remove several unused variables in nsSprocketLayout::PopulateBoxSizes

You can do this via `hg commit --amend`, or `git commit --amend`.
Attachment #8906482 - Flags: review+
Comment hidden (mozreview-request)

Comment 19

2 years ago
mozreview-review
Comment on attachment 8906482 [details]
Bug 1398623 - Remove several unused variables in nsSprocketLayout::PopulateBoxSizes

https://reviewboard.mozilla.org/r/178242/#review183170
Attachment #8906482 - Flags: review+

Comment 20

2 years ago
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/109e5bafd7a3
Remove several unused variables in nsSprocketLayout::PopulateBoxSizes r=xidorn
Sorry, this had to be backed out for asserting during a11y's e.g. accessible/tests/mochitest/states/test_expandable.xul:

https://hg.mozilla.org/integration/autoland/rev/8c9d50f961f55d6c144cbbedfa94a6126ea0dab7

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=109e5bafd7a34423f6ed3ae03fcd277004fa41a5&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=129997515&repo=autoland

TEST-UNEXPECTED-FAIL | accessible/tests/mochitest/events/test_focus_autocomplete.xul | assertion count 2 is more than expected 0 assertions
TEST-UNEXPECTED-FAIL | accessible/tests/mochitest/states/test_expandable.xul | assertion count 1 is more than expected 0 assertions
TEST-UNEXPECTED-FAIL | accessible/tests/mochitest/tree/test_combobox.xul | assertion count 1 is more than expected 0 assertions

Please fix the issue and submit an updated patch. Thank you.
Flags: needinfo?(sajattack)
Hmmm... looks like some of the method calls have side effect of triggering reflow. That's quite unfortunate.
(Reporter)

Comment 23

2 years ago
When I reported the bug, I had a look to the code. I think that the methods are doing changes on the object.
This is why I only mentioned this line being dead code.
It seems GetXULBoxAscent might be the one which has the side effect. Probably we can consider keeping invoking GetXULBoxAscent but just ignore its return value.

I don't think GetXULMargin [1] has any side effect on the layout.

[1] https://searchfox.org/mozilla-central/rev/00fa5dacedb925022f53d025121f1a919508e7ce/layout/xul/nsBox.cpp#352-359
To update the patch, you need to reopen the change in MozReview first.
(Assignee)

Comment 26

2 years ago
I'm not getting the same failure locally so I'm gonna have to push blind and let the trybots work their magic.
Flags: needinfo?(sajattack)
Comment hidden (mozreview-request)

Comment 29

2 years ago
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f0531bd87c9b
Remove several unused variables in nsSprocketLayout::PopulateBoxSizes r=xidorn
https://hg.mozilla.org/mozilla-central/rev/f0531bd87c9b
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox57: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.