Closed Bug 921871 Opened 6 years ago Closed 6 years ago

Fix -Wunitialized warning on Vector ctor

Categories

(Core :: MFBT, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla27

People

(Reporter: reuben, Assigned: reuben)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

Attached patch mfbt-warning (obsolete) — Splinter Review
This warning is triggered for almost every single file in the tree.
Attachment #811728 - Flags: review?(jwalden+bmo)
Ahem, the warning is "field storage is used unitialized here" on the initializer list in the Vector ctor.
That'll fix the warning, certainly.  But I'm not sure we want |storage| moved earlier in the struct, because that gives the C++ compiler less ability to fold together Vector methods that only touch the other fields.  Need to think about this slightly.
Vector seems to be used everywhere, here's a Try run with Talos tests: https://tbpl.mozilla.org/?tree=Try&rev=912f54192e37
I wouldn't look to Talos results to answer this question -- seems somewhat more suited for someone familiar with writing (or at least cognizant of the issues involved in) high-performance template-library sorts of things to consider.

The right solution, it seems to me, might be to not use member-initializer-list initialization for this bit here, but rather to initialize in the body of the constructor, and to initialize mBegin or whichever after storage has been constructed.

Probably we could investigate more and come to some truly-informed conclusion.  But Vector hasn't really been optimized much anyway (even for things like having one concrete implementation for all Vector<T*>), so it seems not likely to provide very meaningful numbers.  So how about just initialize in the constructor, and let's stop worrying about it for now?  :-)
Sounds like a plan!
Assignee: nobody → reuben.bmo
Attachment #811728 - Attachment is obsolete: true
Attachment #811728 - Flags: review?(jwalden+bmo)
Attachment #816097 - Flags: review?(jwalden+bmo)
Comment on attachment 816097 [details] [diff] [review]
Fix warning in Vector.h

Review of attachment 816097 [details] [diff] [review]:
-----------------------------------------------------------------

Seems reasonable.  I'm assuming this fixes the warning, of course, didn't bother testing.  :-)
Attachment #816097 - Flags: review?(jwalden+bmo) → review+
https://hg.mozilla.org/mozilla-central/rev/9ba162545a93
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
Blocks: buildwarning
Target Milestone: mozilla27 → ---
Target Milestone: --- → mozilla27
You need to log in before you can comment on or make changes to this bug.