659210 - new_ Allocators don't support constructors that take reference arguments

Status: RESOLVED WORKSFORME

(Core :: JavaScript Engine, defect)

Description

[David Mandelin [:dmandelin]]

A class with a constructor like this:

  class A {
    B &b;

    A(B &b) : b(b) {}
  };

can't be allocated correctly using the new_ allocators. Currently, it will in fact compile, but the result is that the input argument is copied, and the constructor gets a reference to that copy. This loses; in particular it makes Yarr crash all over the place.

Comment 2

[Nicholas Nethercote [inactive]]

Oh wait, maybe this isn't a dup?

Comment 3

[Paul Biggar]

No, I think this is the exact opposite of bug 657384.

Comment 4

[Paul Biggar]

As I understand it, this is a case of damned-if-you-do, damned-if-you-dont. And I don't think it's possible to fix it to work in both cases.

The "forwarding problem" (http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2002/n1385.htm) is that you can't write a C++ function which perfectly wraps another function, and the examples they give are the same ones we face here.

Option #4 ("const reference + const_cast") is the only option that allows us to use both styles, AIUI. I don't like this one, subtle bugs. We should pick an option that fails statically.

The problem in bug 657384 only affected one call-site. Given that the Yarr thing touches lots of call site, it's probably better to revert bug 657384. I think Yarr has two call-sites that will be affected too (bug 657384 comment 3), so they may need to be uglied.

Also, if we go back to const references, invalid cases will fail statically (albeit with a rubbish error message). Based on comment 0 here, it sounds like the failures are all at run-time, and weird.

So if it sounds good to you dmandelin, I can revert bug 657384 and write an appropriate comment?

Comment 5

[David Mandelin [:dmandelin]]

(In reply to comment #4)
> As I understand it, this is a case of damned-if-you-do, damned-if-you-dont.
> And I don't think it's possible to fix it to work in both cases.
> 
> The "forwarding problem"
> (http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2002/n1385.htm) is that
> you can't write a C++ function which perfectly wraps another function, and
> the examples they give are the same ones we face here.
> 
> Option #4 ("const reference + const_cast") is the only option that allows us
> to use both styles, AIUI. I don't like this one, subtle bugs. We should pick
> an option that fails statically.
> 
> The problem in bug 657384 only affected one call-site. Given that the Yarr
> thing touches lots of call site, it's probably better to revert bug 657384.
> I think Yarr has two call-sites that will be affected too (bug 657384
> comment 3), so they may need to be uglied.
> 
> Also, if we go back to const references, invalid cases will fail statically
> (albeit with a rubbish error message). Based on comment 0 here, it sounds
> like the failures are all at run-time, and weird.
> 
> So if it sounds good to you dmandelin, I can revert bug 657384 and write an
> appropriate comment?

Please don't revert yet. Changing this stuff breaks my landing Yarr, so I'd like to get that done before changing this. The current system actually seems moderately OK to me, and I'm not sure exactly what would improve it. My favorite candidates are:

1. Change the arguments to 'const P1' so that you get a compiler error if the real constructor takes a non-const ref argument. This is basically the current system, and my Yarr patch wouldn't change, but it's safer. It doesn't seem to break anything else but I haven't checked that hard.

2. Change to 'P1 &' and suck up the fact it doesn't work with some rvalue arguments. Not sure how many call sites would need to be changed. If it's a lot, then this seems bad.

3. Change back to 'const P1 &', but spot-add 'P1 &' versions as needed. That's what I did in the yarr patch before bug 657384, and it seemed OK to me. Clearly, adding *all* the possible combinations of const and non-const is too much, but spot-adding seems tolerable.

Comment 6

[Benjamin Bouvier [:bbouvier] (inactive)]

cx->new_ now uses perfect forwarding (yay, C++11), so this shouldn't be an issue anymore. Plus it seems it was dependent on yarr's import, which has been pulled away from the codebase and now we have irregexp, plus it's quite an old bug. Closing as worksforme as I *think* the perfect forwarding allows what's described here, but if that were not the case and needed somehow, we should use a new bug.

http://searchfox.org/mozilla-central/source/js/public/Utility.h#317