Closed
Bug 657384
Opened 13 years ago
Closed 13 years ago
cx->new_ Allocators shouldn't have const ref parameters
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: paul.biggar, Unassigned)
References
Details
(Whiteboard: [fixed-in-tracemonkey])
Attachments
(1 file)
(deleted),
patch
|
luke
:
review+
|
Details | Diff | Splinter Review |
This came up in bug 634711. cx->new_ (and other new_ allocators in jsutil.h) take const reference parameters. This is because they are added by the macros:
template <class T, class P1>\
QUALIFIERS T *new_(const P1 &p1) {\
JS_NEW_BODY(ALLOCATOR, T, (p1))\
}\
That originally came from bug 624878 (adding ::js_new).
However, this breaks calling constructors when you're passing an argument which isn't const. For example:
Oracle::Oracle(Allocator& a) { .. } // constructor, non-const formal parameter
cx->new_<Oracle> (allocator); // assume allocator is non-const
The const is added to allocator by the new_ macro, and the compiler complains because allocator is not const (and should not be). (Actually, I'm surprised there aren't more subtle bugs where we think we're copying when we pass to a constructor, but actually pass by reference).
The attached patch fixes it. Performance on sunspider and v8 is in the noise, both in terms of real performance and instructions executed in valgrind. That's what I'd think would happen, but I didn't want to miss something subtle.
Attachment #532676 -
Flags: review?(luke)
Comment 1•13 years ago
|
||
Comment on attachment 532676 [details] [diff] [review]
Remove const from new_ params
Makes sense. Passing by value is theoretically worrisome, but so far we haven't been giving implicit copy constructors to things that are expensive to copy.
To wit, this was exactly the motivating class of problems for rvalue references in C++0x (http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2002/n1385.htm).
Attachment #532676 -
Flags: review?(luke) → review+
Comment 2•13 years ago
|
||
Is the Oracle case really the only place where a non-const argument is passed to new_()? That seems surprising.
Comment 3•13 years ago
|
||
(In reply to comment #2)
> Is the Oracle case really the only place where a non-const argument is
> passed to new_()? That seems surprising.
New Yarr has 2 ctors that take non-const arguments. But all their other ones apparently don't.
Reporter | ||
Comment 4•13 years ago
|
||
Whiteboard: [fixed-in-tracemonkey]
Comment 5•13 years ago
|
||
cdleary-bot mozilla-central merge info:
http://hg.mozilla.org/mozilla-central/rev/2a76ee0cc5bc
Updated•13 years ago
|
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Reporter | ||
Updated•13 years ago
|
Summary: Allocators should take const ref parameters → cx->new_ Allocators shouldn't have const ref parameters
You need to log in
before you can comment on or make changes to this bug.
Description
•