Bug 19286

Summary: ReadModifyResolveNode(PlacementNewAdoptType) needlessly initializes one member
Product: WebKit Reporter: Peter Siket <pepe>
Component: JavaScriptCoreAssignee: Darin Adler <darin>
Status: RESOLVED FIXED    
Severity: Minor CC: ap, ggaren, mjs
Priority: P3    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   

Description Peter Siket 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        }
Comment 1 Alexey Proskuryakov 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.
Comment 2 Alexey Proskuryakov 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?
Comment 3 Peter Siket 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)
        {
        }
Comment 4 Darin Adler 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.
Comment 5 Darin Adler 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.
Comment 6 Oliver Hunt 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.
Comment 7 Darin Adler 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.
Comment 8 Darin Adler 2008-06-05 09:33:51 PDT
http://trac.webkit.org/changeset/34355