Last Comment Bug 685905 - Add a ReplaceElementAt to nsTArray
: Add a ReplaceElementAt to nsTArray
Product: Core
Classification: Components
Component: XPCOM (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla9
Assigned To: Atul Aggarwal
Depends on:
  Show dependency treegraph
Reported: 2011-09-09 09:47 PDT by Mounir Lamouri (:mounir)
Modified: 2011-09-21 03:01 PDT (History)
3 users (show)
emorley: in‑testsuite-
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Patch v1 (1.29 KB, patch)
2011-09-09 23:45 PDT, Atul Aggarwal
jonas: review+
Details | Diff | Review
Removed the second function as suggested by patch (1.08 KB, patch)
2011-09-17 00:04 PDT, Atul Aggarwal
jonas: review+
Details | Diff | Review
Patch on tip. (1.14 KB, patch)
2011-09-20 08:37 PDT, Atul Aggarwal
no flags Details | Diff | Review

Description Mounir Lamouri (:mounir) 2011-09-09 09:47:34 PDT
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...
Comment 1 Atul Aggarwal 2011-09-09 23:45:09 PDT
Created attachment 559664 [details] [diff] [review]
Patch v1
Comment 2 Jonas Sicking (:sicking) 2011-09-15 10:20:49 PDT
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.
Comment 3 Atul Aggarwal 2011-09-17 00:04:36 PDT
Created attachment 560681 [details] [diff] [review]
Removed the second function as suggested by patch
Comment 4 Jonas Sicking (:sicking) 2011-09-19 12:37:15 PDT
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.
Comment 5 Atul Aggarwal 2011-09-19 20:14:17 PDT
Thanks a lot Jonas. I didn't knew that. I am still learning the procedures.
Comment 6 Ed Morley [:emorley] 2011-09-20 03:21:01 PDT
Doesn't apply to inbound, please update to tip and reattach. Thanks :-)
Comment 7 Atul Aggarwal 2011-09-20 08:37:43 PDT
Created attachment 561202 [details] [diff] [review]
Patch on tip.
Comment 9 Marco Bonardo [::mak] 2011-09-21 03:01:31 PDT

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