Last Comment Bug 327598 - ns*Array::RemoveElementAt should have better return value
: ns*Array::RemoveElementAt should have better return value
Status: RESOLVED FIXED
[mentor=bsmedberg][lang=c++]
:
Product: Core
Classification: Components
Component: XPCOM (show other bugs)
: Trunk
: All All
: P3 normal (vote)
: mozilla23
Assigned To: Sudheera Palihakkara
:
: Nathan Froyd [:froydnj]
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2006-02-16 23:20 PST by Jonas Sicking (:sicking) No longer reading bugmail consistently
Modified: 2013-04-18 08:06 PDT (History)
11 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch file (3.68 KB, patch)
2013-04-13 06:28 PDT, Sudheera Palihakkara
neil: review-
Details | Diff | Splinter Review
Second patch file. rewritten avoiding mistakes. (6.04 KB, patch)
2013-04-13 20:44 PDT, Sudheera Palihakkara
no flags Details | Diff | Splinter Review
3rd patch file. (2.69 KB, patch)
2013-04-16 04:29 PDT, Sudheera Palihakkara
no flags Details | Diff | Splinter Review
4th patch file. (2.56 KB, patch)
2013-04-17 04:59 PDT, Sudheera Palihakkara
neil: review+
Details | Diff | Splinter Review
removed blanks lines and with proper indentation (2.58 KB, patch)
2013-04-17 10:16 PDT, Sudheera Palihakkara
neil: review+
Details | Diff | Splinter Review
patch file ready to be check in (4.25 KB, patch)
2013-04-17 21:51 PDT, Sudheera Palihakkara
ryanvm: checkin+
Details | Diff | Splinter Review

Description Jonas Sicking (:sicking) No longer reading bugmail consistently 2006-02-16 23:20:24 PST
The RemoveElementAt function on nsCOMArray and nsVoidArray currently returns a PRBool indicating if the remove 'succeeded' or not. This is only PR_FALSE if the index was out-of-bounds. The same applies to RemoveElementsAt.

However this is usually not very interesting information. And in makes it seem like error-checking is neccesary when most of the time it is not.

I propose we either make the function return void, or make it return the removed element.

The problem with returning the destroyed element is that RemoveElementAt is implemented as RemoveElementsAt(aIndex, 1), which clearly can't return all destroyed elements.

Any oppinions on which way to go?

I'm going to go over all callsites and see how many would be interested in getting the removed element.

I also propose we make nsTArray do whatever we make nsVoidArray/nsObjectArray do.
Comment 1 Christian :Biesinger (don't email me, ping me on IRC) 2006-02-17 06:46:43 PST
> make it return the removed element.

doesn't that require copying the element, which might be expensive, and which most callers are probably not interested in?
Comment 2 Darin Fisher 2006-02-17 07:02:50 PST
> I also propose we make nsTArray do whatever we make nsVoidArray/nsObjectArray
> do.

nsTArray returns void for RemoveElementsAt.  Keep in mind that nsTArray manages the memory for the elements, so it wouldn't make any sense for an element to exist after it has been removed.
Comment 3 Boris Zbarsky [:bz] (still a bit busy) 2006-02-17 08:45:16 PST
It doesn't make sense for nsCOMArray either, really -- the removal releases, so the element may be dead thereafter.

Are we changing RemoveElement too?  Or just RemoveElementAt?
Comment 4 Jonas Sicking (:sicking) No longer reading bugmail consistently 2006-02-17 11:15:37 PST
Ah, true, for nsTArray it makes sense to do what we do now.
Comment 5 Jonas Sicking (:sicking) No longer reading bugmail consistently 2006-02-17 11:20:30 PST
dagnit, i should make sure i've read all comments before replying :)

Yeah, doing it for nsCOMArray wouldn't be safe either, and so i'd say that just doing it for nsVoidArray is probably a bad idea.

Good question about RemoveElement. Makes a bit more sense to keep the bool there I guess.
Comment 6 Sean Chen 2013-04-11 20:35:57 PDT
Hi, I am currently taking a software engineering class and we saw that this bug would be interesting for us to fix. Thanks
Comment 7 Sudheera Palihakkara 2013-04-12 05:17:24 PDT
Can you assign this to me. I'm this year Gsoc student and trying to do something to get familiar with the source. thanks..!
Comment 8 Marco Bonardo [::mak] 2013-04-12 05:23:49 PDT
Hm, well sorry, I missed comment 6 :(
I suppose the mentor will contact both of you and share the work somehow...
Comment 9 Benjamin Smedberg [:bsmedberg] 2013-04-12 06:11:23 PDT
I think that for now we should just make the function void. Especially with nsCOMArray, having the function return the element would mean that it had to return an already_AddRefed or a nsCOMPtr, and neither of those is the intuitive choice.

Sudheera emailed me about the bug, so it's hers for now.
Comment 10 Sudheera Palihakkara 2013-04-13 06:28:57 PDT
Created attachment 737128 [details] [diff] [review]
patch file
Comment 11 Benjamin Smedberg [:bsmedberg] 2013-04-13 09:37:51 PDT
Comment on attachment 737128 [details] [diff] [review]
patch file

Neil, would you look this over? In particular do we still need/want to keep the silent-success when somebody passes out-of-bounds indexes to this?
Comment 12 neil@parkwaycc.co.uk 2013-04-13 13:02:54 PDT
(In reply to Benjamin Smedberg from comment #11)
> Neil, would you look this over? In particular do we still need/want to keep
> the silent-success when somebody passes out-of-bounds indexes to this?

Fortunately we have very few users of ns(Small)VoidArray left, and they all seem to fall into one of three cases:
1) Remove an item based on calling IndexOf to locate it
2) Remove an item based on a custom loop to identify it
3) Loop over items in descending order, possibly removing them
Comment 13 neil@parkwaycc.co.uk 2013-04-13 13:12:15 PDT
Comment on attachment 737128 [details] [diff] [review]
patch file

The patch appears to contain other errors. I mention them even though the code may get rewritten to remove the bounds checks.

> 		// An invalid index causes the replace to fail
>-		return false;
>+		return ;
Comment is inaccurate, as there is no failure return. Also, please don't leave a space before ;

> bool nsVoidArray::RemoveElement(void* aElement) {
> 	int32_t theIndex = IndexOf(aElement);
> 	if (theIndex != -1)
>-		return RemoveElementAt(theIndex);
>+		RemoveElementAt(theIndex);
>+	return true;
> 
> 	return false;
Code is incorrect.

>@@ -808,37 +809,36 @@
> 		return false;
> 	}
> 
>-	return AsArray()->RemoveElement(aElement);
>+	AsArray()->RemoveElement(aElement);
>+	return true;
This should not have been changed.

>-bool nsSmallVoidArray::RemoveElementAt(int32_t aIndex) {
>+void nsSmallVoidArray::RemoveElementAt(int32_t aIndex) {
> 	if (HasSingle()) {
> 		if (aIndex == 0) {
> 			mImpl = nullptr;
>-
>-			return true;
>+			return ;
This return is no longer necessary.
Comment 14 Sudheera Palihakkara 2013-04-13 20:44:09 PDT
Created attachment 737203 [details] [diff] [review]
Second patch file. rewritten avoiding mistakes.

corrected those error coding.
Comment 15 neil@parkwaycc.co.uk 2013-04-15 16:49:45 PDT
Comment on attachment 737203 [details] [diff] [review]
Second patch file. rewritten avoiding mistakes.

For some reason I am unable to apply this patch to my tree.

>+	if (theIndex != -1){
Note: space between ) and {

>-bool nsSmallVoidArray::RemoveElementsAt(int32_t aIndex, int32_t aCount) {
>+void nsSmallVoidArray::RemoveElementsAt(int32_t aIndex, int32_t aCount) {
> 	if (HasSingle()) {
> 		if (aIndex == 0) {
> 			if (aCount > 0) {
> 				mImpl = nullptr;
> 			}
>-
>-			return true;
>+			return;
Note: I overlooked last time that this return is also now unnecessary.

>-  // bit twiddlers
>+ // bit twiddlers
> void SetArray(Impl *newImpl, int32_t aSize, int32_t aCount);
> 
> private:
>-  /// Copy constructors are not allowed
>+ /// Copy constructors are not allowed
Note: some of the comment lines seem to have been unnecessarily edited.
Comment 16 Sudheera Palihakkara 2013-04-16 04:29:06 PDT
Created attachment 737908 [details] [diff] [review]
3rd patch file.

The comment business was done by eclipse code formatter. Sorry about that. I'm still getting familiar with the code conventions.
Comment 17 neil@parkwaycc.co.uk 2013-04-16 16:32:50 PDT
Comment on attachment 737908 [details] [diff] [review]
3rd patch file.

>diff -r 69bf227607c6 xpcom/glue/nsVoidArray.cpp
Sorry, but I'm still unable to apply the patch. The revision number shown here does not exist in any mozilla repository that I currently have a clone of, nor does the indentation match.
Comment 18 Sudheera Palihakkara 2013-04-17 04:59:37 PDT
Created attachment 738442 [details] [diff] [review]
4th patch file.

I'm sorry, that happend because my lack of experience. Now I have rollback to original source downloaded from mecurial. and  then created the patch file. hope this one would do the trick.
Comment 19 neil@parkwaycc.co.uk 2013-04-17 09:18:14 PDT
Comment on attachment 738442 [details] [diff] [review]
4th patch file.

Excellent, this is much better, and it passes all linux64 tests too.

>-    return RemoveElementAt(theIndex);
>+  {
>+	  RemoveElementAt(theIndex);
>+	  return true;
>+  }
Note: please do not use tabs for indentation.

>     if (aIndex == 0) {
>       mImpl = nullptr;
> 
>-      return true;
>     }
Note: please remove the blank line.

>         mImpl = nullptr;
>       }
> 
>-      return true;
>     }
Note: please remove this blank line too.
Comment 20 Sudheera Palihakkara 2013-04-17 10:16:52 PDT
Created attachment 738595 [details] [diff] [review]
removed blanks lines and with proper indentation

Guess this one do the job.
Comment 21 neil@parkwaycc.co.uk 2013-04-17 11:46:15 PDT
Comment on attachment 738595 [details] [diff] [review]
removed blanks lines and with proper indentation

>-    return RemoveElementAt(theIndex);
>+  {
>+    RemoveElementAt(theIndex);
>+	return true;
>+  }
Still one tab left I'm afraid.

However, this does give me a chance to ask you to follow the instructions in https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F so that the final patch is ready to be checked in.
Comment 22 Sudheera Palihakkara 2013-04-17 21:51:01 PDT
Created attachment 738889 [details] [diff] [review]
patch file ready to be check in

removed that tab. and followed the tutorial and created the patch file.
Comment 23 Ryan VanderMeulen [:RyanVM] 2013-04-18 04:51:23 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/55823b3dbdeb
Comment 24 Ryan VanderMeulen [:RyanVM] 2013-04-18 08:06:56 PDT
https://hg.mozilla.org/mozilla-central/rev/55823b3dbdeb

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