657384 - cx->new_ Allocators shouldn't have const ref parameters

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Paul Biggar]

Attachment: Remove const from new_ params

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.

Comment 1

[Luke Wagner [:luke]]

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).

Comment 2

[Nicholas Nethercote [inactive]]

Is the Oracle case really the only place where a non-const argument is passed to new_()?  That seems surprising.

Comment 3

[David Mandelin [:dmandelin]]

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

Comment 4

[Paul Biggar]

http://hg.mozilla.org/tracemonkey/rev/2a76ee0cc5bc

Comment 5

[Chris Leary [:cdleary] (not checking bugmail)]

cdleary-bot mozilla-central merge info:
http://hg.mozilla.org/mozilla-central/rev/2a76ee0cc5bc