Add a ReplaceElementAt to nsTArray

RESOLVED FIXED in mozilla9

Status

()

Core
XPCOM
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: mounir, Assigned: Atul Aggarwal)

Tracking

Trunk
mozilla9
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

6 years ago
It's already pretty annoying that nsTArray and nsCOMArray do not have the same method signatures but ReplaceElementsAt when we have to replace one element is quite frustrating...
(Assignee)

Comment 1

6 years ago
Created attachment 559664 [details] [diff] [review]
Patch v1
Assignee: nobody → atulagrwl
Attachment #559664 - Flags: review?(jonas)

Updated

6 years ago
Attachment #559664 - Attachment is patch: true
Comment on attachment 559664 [details] [diff] [review]
Patch v1

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

r=me with the second version removed.

::: xpcom/glue/nsTArray.h
@@ +698,5 @@
> +  // A variation on the ReplaceElementsAt method defined above.
> +  template<class Item>
> +  elem_type *ReplaceElementAt(index_type index, const Item* item) {
> +    return ReplaceElementsAt(index, 1, item, 1);
> +  }

I'd rather not have this second signature. Just seems like a source of confusion since people can both pass a pointer to a object, or the object itself.

So just keep the reference version and make people use a deref operator if all they have is a pointer.
Attachment #559664 - Flags: review?(jonas) → review+
(Assignee)

Comment 3

6 years ago
Created attachment 560681 [details] [diff] [review]
Removed the second function as suggested by patch
(Assignee)

Updated

6 years ago
Attachment #560681 - Flags: review?(jonas)
Comment on attachment 560681 [details] [diff] [review]
Removed the second function as suggested by patch

For the record, if someone says "r=me with changes X", you don't really need to re-ask for review. Unless you disagree with the suggested changes or otherwise can't make them.
Attachment #560681 - Flags: review?(jonas) → review+
(Assignee)

Comment 5

6 years ago
Thanks a lot Jonas. I didn't knew that. I am still learning the procedures.
(Assignee)

Updated

6 years ago
Keywords: checkin-needed

Updated

6 years ago
Attachment #559664 - Attachment is obsolete: true

Comment 6

6 years ago
Doesn't apply to inbound, please update to tip and reattach. Thanks :-)
Status: NEW → ASSIGNED
Flags: in-testsuite-
Keywords: checkin-needed
(Assignee)

Comment 7

6 years ago
Created attachment 561202 [details] [diff] [review]
Patch on tip.
Attachment #560681 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/182d4c0c1164
Keywords: checkin-needed
Target Milestone: --- → mozilla9
https://hg.mozilla.org/mozilla-central/rev/182d4c0c1164
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.