Last Comment Bug 657384 - cx->new_ Allocators shouldn't have const ref parameters
: cx->new_ Allocators shouldn't have const ref parameters
Status: RESOLVED FIXED
[fixed-in-tracemonkey]
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: ---
Assigned To: general
:
Mentors:
Depends on:
Blocks: 659210
  Show dependency treegraph
 
Reported: 2011-05-16 09:56 PDT by Paul Biggar
Modified: 2011-05-24 04:26 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Remove const from new_ params (5.00 KB, patch)
2011-05-16 09:56 PDT, Paul Biggar
luke: review+
Details | Diff | Review

Description Paul Biggar 2011-05-16 09:56:46 PDT
Created attachment 532676 [details] [diff] [review]
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] 2011-05-16 10:34:00 PDT
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 [:njn] 2011-05-16 17:53:49 PDT
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] 2011-05-16 18:03:46 PDT
(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 5 Chris Leary [:cdleary] (not checking bugmail) 2011-05-23 14:17:09 PDT
cdleary-bot mozilla-central merge info:
http://hg.mozilla.org/mozilla-central/rev/2a76ee0cc5bc
Comment 6 Nicholas Nethercote [:njn] 2011-05-23 19:33:10 PDT
*** Bug 659210 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.