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

RESOLVED FIXED in Firefox 57

Status

()

Core
XUL
RESOLVED FIXED
2 months ago
2 months 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++])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

2 months ago
http://sylvestre.ledru.info/reports/fx-scan-build/report-nsSprocketLayout.cpp-PopulateBoxSizes-105-1.html#EndPath
ascent += margin.top;
should be removed
(Assignee)

Comment 1

2 months ago
Should I remove the line where ascent is declared as well?
Comment hidden (mozreview-request)
(Assignee)

Comment 3

2 months ago
Created attachment 8906483 [details] [diff] [review]
remove "ascent += margin.top;"

MozReview-Commit-ID: 3VXaVgBhgTL
(Assignee)

Comment 4

2 months ago
Created attachment 8906484 [details] [diff] [review]
my-change.patch
Attachment #8906484 - Flags: 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 months 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 months 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.

Updated

2 months ago
Assignee: nobody → sajattack
Comment hidden (mozreview-request)
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 months 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 months 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 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 months 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 months ago
Attachment #8906492 - Attachment is obsolete: true
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 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 months 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 months 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 months 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)
It seems everything works fine now: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1e9cdf7fa4ab58f63b63196f9218c781ca05d1e7

Comment 29

2 months 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 months 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.