RESOLVED FIXED 19286
ReadModifyResolveNode(PlacementNewAdoptType) needlessly initializes one member
https://bugs.webkit.org/show_bug.cgi?id=19286
Summary ReadModifyResolveNode(PlacementNewAdoptType) needlessly initializes one member
Peter Siket
Reported 2008-05-28 01:40:43 PDT
The following constructor does not initialize the 'm_operator' and the 'm_index' members (rev. 34169): WebKit/JavaScriptCore/kjs/nodes.h(2311): 2311 ReadModifyResolveNode(PlacementNewAdoptType) KJS_FAST_CALL 2312 : ExpressionNode(PlacementNewAdopt) 2313 , m_ident(PlacementNewAdopt) 2314 , m_right(PlacementNewAdopt) 2315 , m_rightHasAssignments(true) 2316 { 2317 }
Attachments
Alexey Proskuryakov
Comment 1 2008-05-28 05:24:03 PDT
I would say that to the contrary, it is a mistake that it initializes a member - this constructor is supposed to be a no-op, only necessary to allocate memory to be later used for placement new.
Alexey Proskuryakov
Comment 2 2008-05-28 06:36:43 PDT
However, simply removing this line results in a 0.5% SunSpider regression (some tests changed by as much as 3%)... I hope it's only gcc randomness! Are these placement new constructors on nodes even necessary still? Could the logic that uses them be moved to emitCode() perhaps?
Peter Siket
Comment 3 2008-05-28 07:25:25 PDT
(In reply to comment #1) > I would say that to the contrary, it is a mistake that it initializes a member > - this constructor is supposed to be a no-op, only necessary to allocate memory > to be later used for placement new. > It is a public constructor. Even though it is used currently only for allocating memory, nobody can guarantee that it will never be used accidentally in the future. And if it happens it could lead to undetermined behaviour. Please also check the other constructor of this class, which does not initialize the 'm_index' member. ReadModifyResolveNode(const Identifier& ident, Operator oper, ExpressionNode* right, bool rightHasAssignm : m_ident(ident) , m_right(right) , m_operator(oper) , m_rightHasAssignments(rightHasAssignments) { }
Darin Adler
Comment 4 2008-05-28 08:56:42 PDT
This does what it's supposed to, but it is unconventional. Luckily, it's obsolete code so we don't have to debate the point. I have a patch that removes this code. It's attached to bug 19269 and should land relatively soon.
Darin Adler
Comment 5 2008-05-28 09:12:21 PDT
I've lowered the priority because there's no real problem here. The concern is entirely theoretical. But soon this code will be history, so as I've said, there's no need to debate it.
Oliver Hunt
Comment 6 2008-05-28 09:36:36 PDT
m_operator and m_index are correctly being reinitialised with their existing values, this is necessary to prevent in place new from overwriting the values of those two fields. Happily this code is now obsolete nyway, so can be removed once we can convince gcc not to make removing this code be a perf regression.
Darin Adler
Comment 7 2008-05-28 09:41:23 PDT
(In reply to comment #6) > Happily this code is now obsolete nyway, so can be > removed once we can convince gcc not to make removing this code be a perf > regression. I have done this and it's not a perf. regression. The patch is attached to bug 19269.
Darin Adler
Comment 8 2008-06-05 09:33:51 PDT
Note You need to log in before you can comment on or make changes to this bug.