891695 - Don't pass pointers by const reference

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[Dan Gohman [:sunfish]]

Attachment: a proposed fix

MDefinition's congruentTo and a few other things pass pointers by const reference, e.g.:

  virtual bool congruentTo(MDefinition* const &ins) const

Pointers are always cheap enough to pass by value, and it's simpler and more idiomatic to do so.

Comment 1

[Ryan Pearl [:rpearl]]

Comment on attachment 773057 [details] [diff] [review]
a proposed fix

Review of attachment 773057 [details] [diff] [review]:
-----------------------------------------------------------------

This looks completely reasonable (although I haven't looked at Mozilla code in about a year now...)

Comment 2

[Dan Gohman [:sunfish]]

Ah, sorry for the trouble then. Thanks for the review! :)

https://hg.mozilla.org/integration/mozilla-inbound/rev/1372b813d76f

Comment 3

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out in http://hg.mozilla.org/integration/mozilla-inbound/rev/d5469522e728 for causing build bustage (warnings as errors) like https://tbpl.mozilla.org/php/getParsedLog.php?id=25355300&tree=Mozilla-Inbound

Comment 4

[Carsten Book [:Tomcat]]

and relanding on inbound https://hg.mozilla.org/integration/mozilla-inbound/rev/56423bbe9a13

Comment 5

[Ryan VanderMeulen [:RyanVM]]

(In reply to Carsten Book [:Tomcat] from comment #4)
> and relanding on inbound
> https://hg.mozilla.org/integration/mozilla-inbound/rev/56423bbe9a13

Not sure why as this still causes the same debug bustage it did last night. Backed out again.
https://hg.mozilla.org/integration/mozilla-inbound/rev/15e6d2cab9f3

Comment 6

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 773057 [details] [diff] [review]
a proposed fix

Review of attachment 773057 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jsapi.h
@@ +2563,3 @@
>  {
>      JSObject *updated = key;
>      JS_SET_TRACING_LOCATION(trc, reinterpret_cast<void *>(&const_cast<JSObject *&>(key)));

This one looks wrong

Comment 7

[Dan Gohman [:sunfish]]

Reapplied without the jsapi.h change:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ad74eb485a87

Comment 8

[Ed Morley [:emorley]]

Backed out for startup crashes:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=ad74eb485a87

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/a984893f3f2a

Comment 9

[Terrence Cole [:terrence]]

Comment on attachment 773057 [details] [diff] [review]
a proposed fix

Review of attachment 773057 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jsapi.h
@@ +2563,3 @@
>  {
>      JSObject *updated = key;
>      JS_SET_TRACING_LOCATION(trc, reinterpret_cast<void *>(&const_cast<JSObject *&>(key)));

Good catch Ms2ger!

Yes, this is wrong: we need to take the (correct) address of key to update the pointer. Normally we pass these by double-pointer to make it clear, hopefully avoiding this easy-to-make mistake. This case is a bit weird because |key| is embedded const in the HashMap:  we don't want to force every user to const-cast it manully.

Comment 10

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/9fed7a1253b1