327598 - ns*Array::RemoveElementAt should have better return value

Status: RESOLVED FIXED

(Core :: XPCOM, defect, P3)

Description

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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)]

> 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]

> 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 [:bzbarsky]]

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]

Ah, true, for nsTArray it makes sense to do what we do now.

Comment 5

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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]

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]

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] (Away Apr 25 - May 5)]

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]

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]

Attachment: patch file

Comment 11

[Benjamin Smedberg]

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

[[e-mail]]

(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

[[e-mail]]

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]

Attachment: Second patch file. rewritten avoiding mistakes.

corrected those error coding.

Comment 15

[[e-mail]]

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]

Attachment: 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

[[e-mail]]

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]

Attachment: 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

[[e-mail]]

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]

Attachment: removed blanks lines and with proper indentation

Guess this one do the job.

Comment 21

[[e-mail]]

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]

Attachment: patch file ready to be check in

removed that tab. and followed the tutorial and created the patch file.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/55823b3dbdeb

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/55823b3dbdeb