Bug 19286 - ReadModifyResolveNode(PlacementNewAdoptType) needlessly initializes one member
Summary: ReadModifyResolveNode(PlacementNewAdoptType) needlessly initializes one member
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P3 Minor
Assignee: Darin Adler
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2008-05-28 01:40 PDT by Peter Siket
Modified: 2008-06-05 09:33 PDT (History)
3 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
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