The default bug view has changed. See this FAQ.

ns*Array::RemoveElementAt should have better return value

RESOLVED FIXED in mozilla23

Status

()

Core
XPCOM
P3
normal
RESOLVED FIXED
11 years ago
4 years ago

People

(Reporter: sicking, Assigned: Sudheera Palihakkara)

Tracking

Trunk
mozilla23
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [mentor=bsmedberg][lang=c++])

Attachments

(1 attachment, 5 obsolete attachments)

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

11 years ago
> 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.
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?
Ah, true, for nsTArray it makes sense to do what we do now.
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.
Assignee: jonas → nobody
Priority: -- → P3
Whiteboard: [mentor=bsmedberg][lang=c++]

Comment 6

4 years ago
Hi, I am currently taking a software engineering class and we saw that this bug would be interesting for us to fix. Thanks
(Assignee)

Comment 7

4 years ago
Can you assign this to me. I'm this year Gsoc student and trying to do something to get familiar with the source. thanks..!

Updated

4 years ago
Assignee: nobody → catchsudheera
Hm, well sorry, I missed comment 6 :(
I suppose the mentor will contact both of you and share the work somehow...
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.
(Assignee)

Comment 10

4 years ago
Created attachment 737128 [details] [diff] [review]
patch file
Attachment #737128 - Flags: review+
(Assignee)

Updated

4 years ago
Attachment #737128 - Flags: review+
(Assignee)

Updated

4 years ago
Attachment #737128 - Flags: review?(benjamin)
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?
Attachment #737128 - Flags: review?(benjamin) → review?(neil)
(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 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.
Attachment #737128 - Flags: review?(neil) → review-
(Assignee)

Comment 14

4 years ago
Created attachment 737203 [details] [diff] [review]
Second patch file. rewritten avoiding mistakes.

corrected those error coding.
Attachment #737128 - Attachment is obsolete: true
Attachment #737203 - Flags: review?(neil)
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.
(Assignee)

Comment 16

4 years ago
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.
Attachment #737203 - Attachment is obsolete: true
Attachment #737203 - Flags: review?(neil)
Attachment #737908 - Flags: review?(neil)
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.
(Assignee)

Comment 18

4 years ago
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.
Attachment #737908 - Attachment is obsolete: true
Attachment #737908 - Flags: review?(neil)
Attachment #738442 - Flags: review?(neil)
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.
Attachment #738442 - Flags: review?(neil) → review+
(Assignee)

Comment 20

4 years ago
Created attachment 738595 [details] [diff] [review]
removed blanks lines and with proper indentation

Guess this one do the job.
Attachment #738595 - Flags: review?(neil)
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.
Attachment #738595 - Flags: review?(neil) → review+
(Assignee)

Comment 22

4 years ago
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.
Attachment #738889 - Flags: checkin?(neil)

Updated

4 years ago
Keywords: checkin-needed
Attachment #738442 - Attachment is obsolete: true
Attachment #738595 - Attachment is obsolete: true
Attachment #738889 - Flags: checkin?(neil) → checkin+
https://hg.mozilla.org/integration/mozilla-inbound/rev/55823b3dbdeb
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/55823b3dbdeb
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
You need to log in before you can comment on or make changes to this bug.