255593 - nsIMutableArray - typo, misleading comment, missing method

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Eyal Rozenberg]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8a2) Gecko/20040714
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8a2) Gecko/20040714

in nsIArray.idl

line 156 reads:
     * @param element Whether or not to store the element using a weak
but should read:
     * @param weak    Whether or not to store the element using a weak

The comment for InsertElementAt mentions the rest of the elements slide forward,
but the comment for RemoveElementAt does not mention the rest of the elements
slide backwards.

I need nsIArray to also have ReplaceElementAt; this is just like Remove+Insert
only saving the trouble of the two slides. It is implemented in nsISupportsArray...

Reproducible: Always
Steps to Reproduce:

Comment 1

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

->me

Comment 2

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Attachment: Patch

Comment 3

[Eyal Rozenberg]

Attachment: patch with comment changes

Why change just one comment at a time?

Comment 4

[Eyal Rozenberg]

Attachment: patch2 + replaceElementAt method

Comment 5

[[e-mail]]

Comment on attachment 156100 [details] [diff] [review]
patch with comment changes

>-     * Insert an element at the given position, and move all elements
>-     * stored at a higher position up one.
>+     * Insert an element at the given position, moing all elements
>+     * currently stored at a same or higher position up one.
moing ;-) Possibly better phrasing would be "moving that element and all
elements stored at a higher position up one."

Comment 6

[Eyal Rozenberg]

Attachment: patch 3 w/ corrected comment

Comment 7

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Comment on attachment 156092 [details] [diff] [review]
Patch

It would seem this patch is now obsolete.

Comment 8

[Darin Fisher]

Comment on attachment 156119 [details] [diff] [review]
patch 3 w/ corrected comment

You must bump the UUID of any interface you modify.  Otherwise, it is nearly
impossible to write code that uses this interface w/ different versions of
Mozilla.

Your documentation for replaceElementAt is lacking comments on the |weak|
parameter.  perhaps you should just start by copying the docs for
insertElementAt.

It would be nice to get alecf's thoughts on this change since he designed this
interface.  Have you tried to get in touch with him?

Comment 9

[Darin Fisher]

cc'ing alecf... (Eyal: though you might want to send him a direct email.)

Comment 10

[Eyal Rozenberg]

Attachment: corrected comments, changed iid

Found another comment issue: for AppendElement, the NS_ERROR_ value is not
NS_ERROR_UNEXPECTED but rather NS_ERROR_FAILURE (like the rest of the functions
in nsArray.cpp). Added the info about the weak param to the comment before
ReplaceElementAt. Assuming that 'bumping' the iid means replacing it with a new
one, then I have done that two. Alec says: "I am on an extended vacation
(currently in Slovenia and leaving for South Africa next week) and won't be
able to do mozilla stuff until I return in January, 2005.. sorry!"

Comment 11

[Eyal Rozenberg]

Comment on attachment 156239 [details] [diff] [review]
corrected comments, changed iid

Hoping for my lucky streak of sr requests from David to continue... this is a
no-brainer fix

Comment 12

[David :Bienvenu]

I'll let Darin give his blessing first :-)

Comment 13

[Darin Fisher]

Comment on attachment 156239 [details] [diff] [review]
corrected comments, changed iid

>Index: xpcom/ds/nsIArray.idl

>-[scriptable, uuid(114744d9-c369-456e-b55a-52fe52880d2d)]
>+[scriptable, uuid(AF059DA0-C85B-40ec-AF07-AE4BFDC192CC)]
> interface nsIArray : nsISupports

shouldn't you be changing the UUID for nsIMutableArray instead?
nsIArray does not appear to have been changed by this patch, so
this particular change is not needed and not advised.


>-     * Insert an element at the given position, and move all elements
>-     * stored at a higher position up one.
>+     * Insert an element at the given position, moving the
>+     * element currently located in that position, and all
>+     * elements in higher position, up by one.

please wrap comments to a line width of 80 characters.


>+    void replaceElementAt(in nsISupports element, in unsigned long index, in boolean weak);

can you document what happens when |index| is out of range?  i.e., 
what if my array has 2 elements and i try to replace the 4th element?
i think this depends on the implementation of the underlying mArray
object, but the behavior should nonetheless be documented here.  same
goes for insertElementAt, i believe.


>Index: xpcom/ds/nsArray.cpp

>+}
>+
>+
>+NS_IMETHODIMP

nit: only use one blank line between function definitions, maybe?

Comment 14

[Eyal Rozenberg]

Attachment: patch v4

Made comments wrap a little bit further, but only as far as the wrapping in the
rest of the file, which is at about 70 chars for comments.

changed the nsIMutableArray UUID instead of the nsIArray UUID (and went 'doh').


wrote a detailed explanation of what happens for various values of index in
InsertElementAt and ReplaceElementAt. The behavior is a bit odd but that's the
way it is.

Removed extra newline.

Comment 15

[Darin Fisher]

Comment on attachment 159879 [details] [diff] [review]
patch v4

>Index: xpcom/ds/nsIArray.idl

>+     * @param index   The position in the array
>+     *                If the position is lower than the current length
>+     *                of the array, an existing element will be

"... will be" -- what? ;-)


>+     *                If the position is equal to the current length
>+     *                of the array, the new element is appended;
>+     *                If the position is higher than the current length

grammar nit: if you end with a ';' then start with a lowercase letter, 
else end with a '.'


thanks for improving the patch, r=darin with nits picked.

Comment 16

[Eyal Rozenberg]

Attachment: v5, fixed nits

Comment 17

[Eyal Rozenberg]

Oh, code was downloaded from 

http://ftp.mozilla.org/pub/mozilla.org/mozilla/nightly/latest/mozilla-source.tar.gz

at about 2004-09-30 13:07 GMT.

Comment 18

[Eyal Rozenberg]

Patch checked in by Neil Rashbrook.