233485 - extend AutoBuffer to have 'size' parameter

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Jungshik Shin]

In bug 161982, AutoBuffer was implemented by Conrad with a single template
parameter. We all agree that it's useful to be able to specify the size of the
stack buffer. So, I'm filing this bug to extend Conrad's implementation.

Comment 1

[[e-mail]]

See also an auto buffer in nsTextTransformer.cpp, this one preserves the buffer
as it reallocates.

Comment 2

[Jungshik Shin]

Yeah, I'm aware of that (see bug 215421 comment #6), but it seems to be a little
too complicated, doesn't it? Most 'consumers' don't need that much.

Comment 3

[[e-mail]]

But then again, most consumers don't reallocate...

Comment 4

[Conrad Carlen (not reading bugmail)]

This comment was from bug 161982 - moving here:

> Conrad, what do you think of combining Ensure..Capacity() and get() into get()
with an optional size parameter as in my implementation?

JShin, I'd like to keep the current semantics, if you're not opposed.
std:auto_ptr and nsCOMPtr have a const get() method. It's a pretty standard
convention. I would like to keep get(), keep it const (i.e., it can't fail), and
separate from Ensure..Capacity.

Comment 5

[Conrad Carlen (not reading bugmail)]

Attachment: adds size template param

Ugh. When I made 2 methods 'const' in response to review comments, I added the
keyword in the wrong place. The template isn't used anywhere yet, so I just
noticed that :-/ Anyway, this patch just adds size as a template param,
preserving the original semantics.

Comment 6

[Jungshik Shin]

Attachment: alternative

Conrad, I'm fine with keeping the semantics. This patch doesn't change that,
but I removed what seems to be unnecessary in attachment 140973 [details] [diff] [review] (and the
current implementation). 

Neil, I was worried about 'template bloat'. What you found in nsTextTransformer
doesn't have a use case elsewhere while all Gfx (Xft, Win, Xlib/Gtk, etc0
consumers need reallocation :-)

Comment 7

[rbs]

Comment on attachment 141004 [details] [diff] [review]
alternative

A couple of suggestions:

1) remove DeleteBuffer() and just inline its one-liner code... [it avoids undue
confuison. I couldn't really tell what |DeleteBuffer| does without looking at
its code.]

2) why not just say 'EnsureCapacity' rather than 'EnsureElemCapacity'.

3) Add some commented examples of usage in the header (e.g., borrowing from
Gfx:Win...)

typedef nsAutoArray<PRUint8, AUTO_FONTDATA_BUFFER_SIZE> nsAutoFontDataBuffer;
typedef nsAutoArray<char, CHAR_BUFFER_SIZE> nsAutoCharBuffer;
typedef nsAutoArray<PRUnichar, CHAR_BUFFER_SIZE> nsAutoChar16Buffer;

Comment 8

[Jungshik Shin]

What a coincidence ! I was just thinking about exactly the same things (except
for #3) :-) It seems like we can do 's/Elem//g'.

Comment 9

[Conrad Carlen (not reading bugmail)]

> 2) why not just say 'EnsureCapacity' rather than 'EnsureElemCapacity'.

The 'Elem' part was suggested in reviews of the original code to remind people
that it was in terms of elements, not bytes. I agreed.

Comment 10

[rbs]

I suspected that. Probably late in the game... I would have called it
nsAutoArray, GetCount(), EnsureCount(), as 'array' makes it clear that it is
element-based, without requiring further qualification.

Comment 11

[[e-mail]]

(In reply to comment #6)
> Neil, I was worried about 'template bloat'.
T* newBuffer = (T*)nsMemory::Alloc(inElemCapacity * sizeof(T));
would become
T* newBuffer;
if (mBufferPtr != mStackBuffer)
  newBuffer = (T*)nsMemory::Realloc(mBufferPtr, inElemCapacity * sizeof(T));
else
  newBuffer = (T*)nsMemory::Alloc(inElemCapacity * sizeof(T));
If you're that worried you could put the code in a base class.

Comment 12

[Jungshik Shin]

Oh, that's simple enough. I thought you wanted to implement what's implemented at

 http://lxr.mozilla.org/mozilla/source/layout/html/base/src/nsTextTransformer.h#100

with |PRBool aCopyToHead| in GrowTo/GrowBy.

Comment 13

[[e-mail]]

It comes to the same thing... an AddCapacity method might be nice though.

Comment 14

[Jungshik Shin]

Attachment: update

I added reallocation and AddElemCapacity.

Comment 15

[Jungshik Shin]

Attachment: patches for 'customers' of nsAutoBuffer

I haven't included nsTextFrame and nsTextTransformer because 
the buffer needs to zero'd (with memset).

Comment 16

[Jungshik Shin]

Comment on attachment 141164 [details] [diff] [review]
update


>+    if (mBufferPtr != mStackBuffer)
>+      newBuffer = (T*)nsMemory::Realloc(inElemCapacity * sizeof(T));

  Oops. This should be 

  ... ::Realloc((void *)mBufferPtr, inElemCapacity * sizeof(T));

Comment 17

[rbs]

Looks good and quite neat and tidy to me, modulo your errata.

Comment 18

[Jungshik Shin]

Comment on attachment 141164 [details] [diff] [review]
update

asking for r/sr.

rbs, can you take a look at the 'customer patch' as well?

Comment 19

[rbs]

I had looked at it, and it is good too. I will sr= once you are done with the r=.

Comment 20

[Conrad Carlen (not reading bugmail)]

Comment on attachment 141164 [details] [diff] [review]
update

Looks good. r=ccarlen.

I'd rather see a more generic example than one from nsFontMetricsWin.cpp. Also,
one that shows buffer.get() having data copied into it.

Comment 21

[rbs]

Comment on attachment 141164 [details] [diff] [review]
update

sr=rbs

Comment 22

[rbs]

Comment on attachment 141166 [details] [diff] [review]
patches for 'customers' of nsAutoBuffer

sr=rbs, on this too, while I am here.

Comment 23

[Jungshik Shin]

Comment on attachment 141166 [details] [diff] [review]
patches for 'customers' of nsAutoBuffer

Thanks for r/sr. Conrad, can you review OSX part and put r stamp on it? I think
for gfx changes, rbs' sr should suffice.

Comment 24

[Jungshik Shin]

Oops. I missed Conrad's review comment. Would something like the following work? 

typedef nsAutoBuffer<PRUnichar, 256> nsAutoUnicharBuffer;

nsAutoUnicharBuffer buffer;

if (!buffer.EnsureElemCapacity(initialLength)) return NS_ERROR_OUT_OF_MEMORY;
PRUnichar *unicharPtr = buffer.get();
// add PRUnichar's to the buffer pointed to by |unicharPtr| as long as
// the number of PRUnichar's is less than |intialLength|

// increase the capacity
if (!buffer.AddElemCapacity(extraLength)) return NS_ERROR_OUT_OF_MEMORY
unicharPtr = buffer.get() + initialLength;
// continue to add PRUnichar's....

Comment 25

[Conrad Carlen (not reading bugmail)]

Comment on attachment 141166 [details] [diff] [review]
patches for 'customers' of nsAutoBuffer

>Index: xpcom/io/nsLocalFileOSX.cpp
>===================================================================
>-  
>-protected:
>-  enum { kStackBufferNumElems		= 512 };
>-
>-  T             *mBufferPtr;
>-  T             mBuffer[kStackBufferNumElems];
>-  PRInt32       mCurElemCapacity;
>-};
>-
>+#define FILENAME_BUFFER_SIZE 256

I'd leave this at 512. PATH_MAX == 1024 on OS X, so 1/2 that seemed to be a
good value.

> nsresult nsLocalFile::CFStringReftoUTF8(CFStringRef aInStrRef, nsACString& aOutStr)
> {
>   nsresult rv = NS_ERROR_FAILURE;
>   CFIndex usedBufLen, inStrLen = ::CFStringGetLength(aInStrRef);
>   CFIndex charsConverted = ::CFStringGetBytes(aInStrRef, CFRangeMake(0, inStrLen),
>                               kCFStringEncodingUTF8, 0, PR_FALSE, nsnull, 0, &usedBufLen);
>   if (charsConverted == inStrLen) {
>-    StBuffer<UInt8> buffer;
>+    nsAutoBuffer<UInt8, FILENAME_BUFFER_SIZE * 2> buffer;

Is this going to cause a whole new instantiation for this size? If so, I'd just
make FILENAME_BUFFER_SIZE bigger (512) and use only that value.

>     if (buffer.EnsureElemCapacity(usedBufLen + 1)) {
>       ::CFStringGetBytes(aInStrRef, CFRangeMake(0, inStrLen),
>           kCFStringEncodingUTF8, 0, false, buffer.get(), usedBufLen, &usedBufLen);
>       buffer.get()[usedBufLen] = '\0';
>       aOutStr.Assign(nsDependentCString((char*)buffer.get()));
>       rv = NS_OK;

r=ccarlen with those addressed.

Comment 26

[Conrad Carlen (not reading bugmail)]

(In reply to comment #24)
> Oops. I missed Conrad's review comment. Would something like the following work? 

Looks good.

Comment 27

[Jungshik Shin]

fix checked into the trunk with Conrad's concerns addressed.

Comment 28

[[e-mail]]

This seems to have caused build bustage on MacOSX Darwin 6.6 monkey Dep %

Comment 29

[Jungshik Shin]

bustage was fixed. thanks, neil, for the note.

Comment 30

[Jungshik Shin]

*** Bug 215421 has been marked as a duplicate of this bug. ***