161982 - Need an "auto buffer" impl available to all code.

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Conrad Carlen (not reading bugmail)]

Try this search:
http://lxr.mozilla.org/seamonkey/search?string=nsSpillableStackBuffer

We have 2 implementations of the same thing. I have a patch in which I needed
yet another. What we need instead is to have this in XPCOM, for anybody to use.
Along those lines, I made it a template class so the data it stores is typed.
Patch coming up...

Comment 1

[Conrad Carlen (not reading bugmail)]

Attachment: template implementation

Here it is again, as a template. scc - can I do this. I'm not sure is templates
fit in with our portability guidelines, but then nsCOMPtr is a template. Also,
this template usage is so basic, I would think it should fly on all supported
compilers.

Comment 2

[Conrad Carlen (not reading bugmail)]

Comment on attachment 94674 [details] [diff] [review]
template implementation

This could be useful for bug 202014.

Comment 3

[Kathleen :Brade]

Comment on attachment 94674 [details] [diff] [review]
template implementation

nsAutoBuffer.h has NPL1.1; it should be MPL (find all occurrences of NPL).  Is
the copyright year correct?  Do you want to change your e-mail address?

Does this build on all platforms?  Which platforms have you tested it on?  Why
add the header file to the makefile if you aren't planning to use it (or is
that in another bug)?  I'd like to see more differentiation between these
names:	mBuffer and mBufferPtr.  Perhaps mStackBuffer or ?

r=brade

Comment 4

[Brian Ryner (not reading)]

Comment on attachment 94674 [details] [diff] [review]
template implementation

>Index: nsAutoBuffer.h
>===================================================================
>RCS file: nsAutoBuffer.h
>diff -N nsAutoBuffer.h
>--- /dev/null	1 Jan 1970 00:00:00 -0000
>+++ nsAutoBuffer.h	9 Aug 2002 20:38:31 -0000
>+  PRInt32     GetElemCapacity()   { return mCurElemCapacity;  }
>+                
>+  T*          get()     { return mBufferPtr;    }

These two methods can be |const|.

Other than that, sr=bryner (with brade's comments addressed).

Comment 5

[Conrad Carlen (not reading bugmail)]

Checked in - sorry, I had completely forgotten about this one :-/

I'll file a follow-up bug to convert uses of nsSpillableStackBuffer and the
internal impl used by nsLocalFileOSX to use the new code.

Comment 6

[[e-mail]]

This buffer appears to be doing more work than is necessary for the current
users... indeed, calling EnsureCapacity more than once does not appear to be
useful, something along these lines would appear to suffice:
template <class T>
class nsAutoBuffer
{
public:
  nsAutoBuffer(PRInt32 capacity) {
    if (capacity > kStackElems)
      mBufferPtr = new T[capacity];
    else
      mBufferPtr = mStackBuffer;
  }
  ~nsAutoBuffer() {
    if (mBufferPtr != mStackBuffer)
      delete mBufferPtr;
  }
  T* get() { return mBufferPtr; } const;
protected:
  enum { kStackElems = 256 };
  T *mBufferPtr;
  T mStackBuffer[kStackElems];
}
Usage:
nsAutoBuffer<T> buffer(aLen);
if (!buffer.get())
  return NS_ERROR_OUT_OF_MEMORY;

Comment 7

[Jungshik Shin]

My implementation is similar to Conrad's with a second template parameter for
the static buffer size. In Gfx, '256' is too small while for others , that's
large enough so that we need a template parameter for the size. Moreover,
'GetArray' method does double as Conrad's 'Ensure...' and 'Get....' in my
implementation. For my usage in Gfx, it's more convenient that way. 

See 

http://lxr.mozilla.org/seamonkey/source/gfx/src/windows/nsFontMetricsWin.cpp#577

and a non-template version at 
http://lxr.mozilla.org/seamonkey/source/gfx/src/gtk/nsFontMetricsXft.cpp#304
See also bug 215421 comment #6 for potential beneficiaries and bug 70870

Comment 8

[[e-mail]]

Ok, so you do need a slightly more complex buffer...

Comment 9

[Conrad Carlen (not reading bugmail)]

> calling EnsureCapacity more than once does not appear to be useful

It allows you to create one of these and then reuse it for a number of different
operations in one scope, each having a different size. If the size is specified
in the contructor, it can only be used for one operation.

But, I do like the feature of the impl here:
http://lxr.mozilla.org/seamonkey/source/gfx/src/windows/nsFontMetricsWin.cpp#577
in that a size is part of the template. Hardcoding 256 as the stack buffer size
is the weakness of mine.

Comment 10

[Jungshik Shin]

Conrad, what do you think of combining Ensure..Capacity() and get() into get()
with an optional size parameter as in my implementation? When 'size' is not
specified, it just works like the current 'get()'. With 'size', it allocates a
new buffer if necessary and returns the pointer. Null-checking the returned ptr
is equivalent to the boolean checking on Ensure...(). It seems to me it's a
little simpler that way.
On the other hand, it's less clear 'symantically'.

Comment 11

[rbs]

jshin, why don't you bite the bullet and move your generic template
implementation in the new file that conrad has created? You have been thinking
of doing that for some time... It is quite useful to be able to specify the size
of the stack buffer.

There might be some cross-platform hassles and hiccups, but as the hashtable
stuff has shown, these hiccups can be calmed down, provided enough perseverance
and perspiration, and you seem not to be afraid of such things :-) Plus, it is a
good sign that conrad's template has left all trees green. If you are do to so,
you can open another bug for those of us who are interested.

Comment 12

[Jungshik Shin]

OK. I'm biting the bullet. bug 233485 has been just filed.