Closed Bug 161982 Opened 22 years ago Closed 21 years ago

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

Categories

(Core :: XPCOM, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: ccarlen, Assigned: ccarlen)

Details

Attachments

(1 file)

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...
Attached patch template implementation (deleted) — Splinter Review
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 on attachment 94674 [details] [diff] [review] template implementation This could be useful for bug 202014.
Attachment #94674 - Flags: superreview?(bryner)
Attachment #94674 - Flags: review?(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
Attachment #94674 - Flags: review?(brade) → review+
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).
Attachment #94674 - Flags: superreview?(bryner) → superreview+
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.
Status: NEW → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
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;
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
Ok, so you do need a slightly more complex buffer...
> 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.
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'.
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.
OK. I'm biting the bullet. bug 233485 has been just filed.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: