Don't pass pointers by const reference

RESOLVED FIXED in mozilla25

Status

()

--
enhancement
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: sunfish, Assigned: sunfish)

Tracking

Trunk
mozilla25
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

5 years ago
Created attachment 773057 [details] [diff] [review]
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.
Attachment #773057 - Flags: review?(rpearl)

Comment 1

5 years ago
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...)
Attachment #773057 - Flags: review?(rpearl) → review+
(Assignee)

Comment 2

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/1372b813d76f
(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 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
(Assignee)

Comment 7

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

Updated

5 years ago
Assignee: general → sunfish
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.
https://hg.mozilla.org/mozilla-central/rev/9fed7a1253b1
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
You need to log in before you can comment on or make changes to this bug.