WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Add attachment
proposed patch, testcase, etc.
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
http://trac.webkit.org/changeset/34355
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug