Bug 19286 – ReadModifyResolveNode(PlacementNewAdoptType) needlessly initializes one member

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Peter Siket]

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        }

[Alexey Proskuryakov]

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]

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]

(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]

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]

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]

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]

(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]

http://trac.webkit.org/changeset/34355