Closed Bug 1398623 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: XUL, defect)

Product:
Core
Component:
XUL
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla57
Tracking Flags:
Tracking Status
firefox57 --- fixed

People

(Reporter: Sylvestre, Assigned: sajattack, Mentored)

References

(Blocks 1 open bug)

Details

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

Attachments

(1 file, 3 obsolete files)

Bug 1398623 - Remove several unused variables in nsSprocketLayout::PopulateBoxSizes
59 bytes, text/x-review-board-request
Details
Should I remove the line where ascent is declared as well?
Attached patch remove "ascent += margin.top;" (obsolete) — — Splinter Review
MozReview-Commit-ID: 3VXaVgBhgTL
Attached patch my-change.patch (obsolete) — — Splinter Review
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?
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.
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 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
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.
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.
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.
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 on attachment 8906482 [details] Bug 1398623 - Remove several unused variables in nsSprocketLayout::PopulateBoxSizes https://reviewboard.mozilla.org/r/178242/#review183170
Attachment #8906482 - Flags: review+
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.
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.
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)
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
Closed: 7 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.

Attachment

General

Creator:
Created:
Updated:
Size: