Closed Bug 1445216 Opened 7 years ago Closed 7 years ago

nsPlainTextSerializer::Write: Value stored to 'atFirstColumn' during its initialization is never read

Categories

(Core :: DOM: Core & HTML, enhancement, P2)

Product:
Core
Component:
DOM: Core & HTML
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla63
Tracking Flags:
Tracking Status
firefox63 --- fixed

People

(Reporter: Sylvestre, Assigned: petru.gurita1, Mentored)

References

(Blocks 1 open bug)

Details

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

Attachments

(1 file, 1 obsolete file)

Bug 1445216 - remove useless initialization of variable `atFirstColumn` in nsPlainTextSerializer::Write.
59 bytes, text/x-review-board-request
bzbarsky
: review+
Details
http://sylvestre.ledru.info/reports/fx-scan-build/report-nsPlainTextSerializer.cpp-Write-51-1.html#EndPath We could replace bool atFirstColumn = mAtFirstColumn; by = true; And remove the lone 1683: atFirstColumn = true;
Hi, I'm interested to fix this issue.
Assignee: nobody → matthiou.75
Priority: -- → P2
Matthieu, you will need to find a review now! https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Introduction#Step_4_-_Get_your_code_reviewed
Flags: needinfo?(matthiou.75)
Attachment #8958647 - Flags: review?(ehsan)
Attachment #8958647 - Flags: review?(bzbarsky)
Comment on attachment 8958647 [details] Bug 1445216 - nsPlainTextSerializer::Write: Value stored to 'atFirstColumn' during its initialization is never read No need to ask two reviewer. One is enough!
Attachment #8958647 - Flags: review?(ehsan)
Attachment #8958647 - Flags: review?(bzbarsky) → review?(ehsan)
Comment on attachment 8958647 [details] Bug 1445216 - nsPlainTextSerializer::Write: Value stored to 'atFirstColumn' during its initialization is never read https://reviewboard.mozilla.org/r/227564/#review235182 I think this actually makes the logic a lot more confusing. Would be clearer to leave the value uninitialized up top and then set it on both branches of the if as we do now...
Attachment #8958647 - Flags: review?(ehsan)
OK, my bad! Matthieu, could you please take in account Boris's comment? Thanks!
Seems that Matthieu doesn't work on it anymore. Available for someone else!
Assignee: matthiou.75 → nobody
Keywords: good-first-bug
Whiteboard: [good first bug][lang=C++]
Can I work on this too? :)
Feel free to. Should I assign this bug to you?
I want to work on this issue?
Flags: needinfo?(sledru)
I will assign it to the first person who provides a patch.
Flags: needinfo?(sledru)
I'm interested to fix this issue.
Hello, I think I successfully provided a patch attachment 253432 [details] [diff] [review]
Seems that you are not doing what Boris said in comment #5 ?
Flags: needinfo?(matthiou.75)
(In reply to Petru Gurita from comment #14) > Hello, I think I successfully provided a patch attachment 253432 [details] [diff] [review] > [diff] [review] Wrong attachment link. Sorry. attachment 8988180 [details]
(In reply to Sylvestre Ledru [:sylvestre] from comment #15) > Seems that you are not doing what Boris said in comment #5 ? I did. I let it uninitialized up top. It is already set on both branches, so I don't think there is anything else to add.
Attachment #8958647 - Attachment is obsolete: true
Assignee: nobody → petru.gurita1
status-firefox61: affected → ---
Comment on attachment 8988180 [details] Bug 1445216 - remove useless initialization of variable `atFirstColumn` in nsPlainTextSerializer::Write. OK, right, sorry. You should also update the commit message to explain what you are doing. Like "Remove the initialization as the foo variable is set in both branches" or something like that.
Attachment #8988180 - Flags: review?(sledru) → feedback+
(In reply to Sylvestre Ledru [:sylvestre] from comment #18) > Comment on attachment 8988180 [details] > Bug 1445216 - nsPlainTextSerializer::Write: Value stored to 'atFirstColumn' > during its initialization is never read; > > OK, right, sorry. > You should also update the commit message to explain what you are doing. > Like "Remove the initialization as the foo variable is set in both branches" > or something like that. I've updated the commit message accordingly.
Comment on attachment 8988180 [details] Bug 1445216 - remove useless initialization of variable `atFirstColumn` in nsPlainTextSerializer::Write. Sounds good but I cannot review this patch as I am not the component owner!
Attachment #8988180 - Flags: review?(sledru) → feedback+
I'm sorry. I've requested a review from the component owner (peterv). I'm waiting for his response. I'll now move on to more challenging bugs :) .
You should probably ask bz as he was involved before: https://bugzilla.mozilla.org/show_bug.cgi?id=1445216#c5
Comment on attachment 8988180 [details] Bug 1445216 - remove useless initialization of variable `atFirstColumn` in nsPlainTextSerializer::Write. https://reviewboard.mozilla.org/r/253432/#review260428 ::: commit-message-34809:1 (Diff revision 4) > +Bug 1445216 - nsPlainTextSerializer::Write: Value stored to 'atFirstColumn' during its initialization is never read; r?bz The first line of the commit message should describe the change being made, not just copy the bug summary. So something like: Bug 1445216 - remove useless initialization of atFirstColumn in nsPlainTextSerializer::Write. r?bz and then a blank line, and then: The atFirstColumn variable is set in both branches of an "if" before it's read. r=me with that. Thank you!
Attachment #8988180 - Flags: review?(bzbarsky) → review+
Hello, it's done.
Pushed by bzbarsky@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/803ff4b978c4 remove useless initialization of variable `atFirstColumn` in nsPlainTextSerializer::Write. r=bz
Petru, thank you for fixing this!
https://hg.mozilla.org/mozilla-central/rev/803ff4b978c4
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox63: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: