Closed Bug 1361647 Opened 3 years ago Closed 3 years ago

Coverity report: nsMathMLmtableFrame::​nsMathMLmtableFrame(nsStyleContext *): A scalar field is not initialized by the constructor

Categories

(Core :: MathML, defect, P3)

53 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox-esr52 --- unaffected
firefox53 --- wontfix
firefox54 --- wontfix
firefox55 --- fixed

People

(Reporter: mats, Assigned: fredw)

References

(Blocks 1 open bug, )

Details

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

Attachments

(1 file)

Coverity CID 1225549 Uninitialized scalar field

The field will contain an arbitrary value left over from earlier computations.

In nsMathMLmtableFrame::​nsMathMLmtableFrame(nsStyleContext *): A scalar field is not initialized by the constructor 



159protected:
160  explicit nsMathMLmtableFrame(nsStyleContext* aContext)
161    : nsTableFrame(aContext)
   2. uninit_member: Non-static class member mFrameSpacingX is not initialized in this constructor nor in any functions that it calls.
   4. uninit_member: Non-static class member mFrameSpacingY is not initialized in this constructor nor in any functions that it calls.
   CID 1225549 (#1 of 1): Uninitialized scalar field (UNINIT_CTOR)6. uninit_member: Non-static class member mUseCSSSpacing is not initialized in this constructor nor in any functions that it calls.
162  {}
Whiteboard: [lang=C++] → [lang=C++][CID 1225549]
Attached patch PatchSplinter Review
Assignee: nobody → fred.wang
Status: NEW → ASSIGNED
Attachment #8865109 - Flags: review?(karlt)
Comment on attachment 8865109 [details] [diff] [review]
Patch

I have always preferred to initialize variables only once when that is practical.
In the constructor here, no attempt is made to set these to the correct values and so they need to be set again later.

They are not used before set correctly, but setting incorrect values before the correct values are calculated can hide from tools such as valgrind a failure to set correct values.

Perhaps it is possible to call MapAllAttributesIntoCSS() from the constructor.  I don't know, but I also don't know why it is important to do so.

Perhaps Mats knows why Coverity thinks these should be set in the constructor.
Attachment #8865109 - Flags: review?(karlt) → review?(mats)
> Perhaps Mats knows why Coverity thinks these should be set in the constructor.

Because it's a footgun to have uninitialized members laying around.
Comment on attachment 8865109 [details] [diff] [review]
Patch

LGTM, thanks!
Attachment #8865109 - Flags: review?(mats) → review+
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/84a52fd86062
Coverity report: nsMathMLmtableFrame::nsMathMLmtableFrame(nsStyleContext *): A scalar field is not initialized by the constructor. r=karlt
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/84a52fd86062
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.