335957 - Provide method to resize-and-get mutable string data

Status: RESOLVED FIXED

(Core :: XPCOM, enhancement, P2)

Description

[Darin Fisher]

Provide method to resize-and-get mutable string data

Currently, callers must do something like this:

  str.SetLength(len);
  if (str.Length() != len)
    return NS_ERROR_OUT_OF_MEMORY;
  char *data = str.BeginWriting();

Or, if they do not wish to change the string buffer's size:

  char *data = str.BeginWriting();

In this latter case, OOM error handling is difficult since the API promises to never return null.  If the string's buffer was immutable prior to the call and if there is not enough memory to make a copy of the immutable buffer, then it will just happily return the immutable buffer.

I propose that we make BeginWriting take an optional size parameter and have it return null in out-of-memory cases.

Comment 1

[Darin Fisher]

Attachment: prototype patch: proposed interfaces

dbaron: please review this API proposal.  this is not a final patch for check-in.  if you are happy with these API changes, then I will go about fixing consumers in the tree to make use of this API.

Comment 2

[Darin Fisher]

I added GetData and GetMutableData methods that are similar to NS_StringGetData and NS_StringGetMutableData.  I added those because they provide the resulting length in addition to the buffer pointer.

I chose not to make SetLength return a boolean since then I would be encouraged to make Truncate return a boolean, and then Cut, and then Replace, and then just about every other mutation method.  However, I cannot add such return values for operator= and operator+=, so it would be inconsistent.  Plus, I think the current design of making the string API hide low-memory errors is okay.  The only time when it is not okay is when lack of error checking could result in crashes and such, which is only the case for BeginWriting.

Comment 3

[Benjamin Smedberg]

> Or, if they do not wish to change the string buffer's size:
> 
>   char *data = str.BeginWriting();
> 
> In this latter case, OOM error handling is difficult since the API promises to
> never return null.  If the string's buffer was immutable prior to the call and

It does? Where? IMO we should just make it return nsnull in OOM conditions (I recently added this API, so I think we can/should make this change now without breaking the world.

Also, we could add an version of the same API to change-length and write:

char_type* BeginWriting(PRUint32 aNewLength);

In addition I think we should support the shortcut API I added for the frozen string API:

PRUint32 /*length*/ BeginWriting(char_type **start, char_type **end = nsnull,
                                 PRUint32 aNewLength = PR_UINT32_MAX);

Comment 4

[Darin Fisher]

What about the BeginWriting that returns an iterator?  Does it return an iterator that points to null?

Comment 5

[Benjamin Smedberg]

I don't know, changing that could have affects all over our codebase. Perhaps we should just make the new API correct and encourage people to switch over time?

Comment 6

[Darin Fisher]

> What about the BeginWriting that returns an iterator?  
> Does it return an iterator that points to null?

Perhaps it returns an iterator that returns 0 for size_forward().

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 220261 [details] [diff] [review]
prototype patch: proposed interfaces

I'm not a big fan of the changes to the BeginWriting and EndWriting signatures, but perhaps somebody wants to use them rather than GetMutableData.  I didn't look closely at the nsTAString changes.  But other than that this seems fine.

Could you make the comments in the definition of SetLength clearer what its effects on the capacity of the string are?  Does it shrink the capacity in some cases?

I also wish SetLength and SetCapacity had comments making in clear that the size for the null terminator does not need to be included (that's right, isn't it?).

Comment 8

[Darin Fisher]

Right, the capacity does not include the null terminator.  I added a comment to SetCapacity to clarify that.  SetLength doesn't need it because it says that the given length value specifies the offset of the null terminator.

I removed the newLen parameter from BeginWriting because I think it isn't needed.  It can be added later if I'm wrong.

Comment 9

[Darin Fisher]

Attachment: v1.1 patch

here's the version of the patch that i committed.

Comment 10

[Darin Fisher]

fixed