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.

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

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.

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

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.

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.

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

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

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

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.

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

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.

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

I added reallocation and AddElemCapacity.

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

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

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

Comment on attachment 141164 [details] [diff] [review] update asking for r/sr. rbs, can you take a look at the 'customer patch' as well?

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

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 on attachment 141164 [details] [diff] [review] update sr=rbs

Comment on attachment 141166 [details] [diff] [review] patches for 'customers' of nsAutoBuffer sr=rbs, on this too, while I am here.

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.

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

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

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

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

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

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