Re: [PATCH V2 3/3] strbuf: allow to use preallocated memory

Jeff King writes: > I agree it can be confusing (especially on the output side your errors > are likely deferred until the next flush). But in this particular case, > I think it's an improvement (see the patch I just sent and its > discussion of error handling). Yes, for this

Re: [PATCH V2 3/3] strbuf: allow to use preallocated memory

On Wed, Jun 08, 2016 at 12:48:36PM -0700, Junio C Hamano wrote: > Jeff King writes: > > > That made me wonder if we could repeatedly reuse a buffer attached to > > the file descriptor. And indeed, isn't that what stdio is? The whole > > reason this buffer exists is because we are

Re: [PATCH V2 3/3] strbuf: allow to use preallocated memory

Jeff King writes: > That made me wonder if we could repeatedly reuse a buffer attached to > the file descriptor. And indeed, isn't that what stdio is? The whole > reason this buffer exists is because we are using a direct descriptor > write. If we switched this function to use

Re: [PATCH V2 3/3] strbuf: allow to use preallocated memory

On Wed, Jun 08, 2016 at 06:20:41PM +0200, Michael Haggerty wrote: > Instead, one could write > > > static int feed_object(const unsigned char *sha1, int fd, int negative) > > { > > char buf[GIT_SHA1_HEXSZ + 2]; > > struct strbuf line = WRAPPED_FIXED_STRBUF(buf); > > > > if (negative

Re: [PATCH V2 3/3] strbuf: allow to use preallocated memory

Michael Haggerty writes: > * It's a little less manual bookkeeping, and thus less error-prone, > than the current code. > > * If somebody decides to add another character to the line but > forgets to increase the allocation size, the code dies in testing > rather than

Re: [PATCH V2 3/3] strbuf: allow to use preallocated memory

On 06/07/2016 11:06 AM, William Duclot wrote: > [...] > The "fixed" feature was aimed to allow the users to use strbuf with > strings that they doesn't own themselves (a function parameter for > example). From Michael example in the original mail: > > void f(char *path_buf, size_t path_buf_len) >

Re: [PATCH V2 3/3] strbuf: allow to use preallocated memory

William Duclot writes: >> Perhaps I made it clearer by using a more exaggerated example e.g. a >> typical expected buffer size of 128 bytes, but the third line of >> 1000 line input file was an anomaly that is 200k bytes long. I do >> not want to keep

Re: [PATCH V2 3/3] strbuf: allow to use preallocated memory

On Mon, Jun 06, 2016 at 04:24:53PM -0700, Junio C Hamano wrote: > Jeff King writes: > >>> I think that call should reset line.buf to the original buffer on >>> the stack, instead of saying "Ok, I'll ignore the original memory >>> not owned by us and instead keep pointing at the

Re: [PATCH V2 3/3] strbuf: allow to use preallocated memory

On Mon, Jun 06, 2016 at 04:24:53PM -0700, Junio C Hamano wrote: > This is not about stack vs heap or even "cheaper" (whatever your > definition of cheap is). The principle applies equally if the > original buffer came from BSS. > > Perhaps I made it clearer by using a more exaggerated example

Re: [PATCH V2 3/3] strbuf: allow to use preallocated memory

Junio C Hamano writes: > Jeff King writes: > >>> I think that call should reset line.buf to the original buffer on >>> the stack, instead of saying "Ok, I'll ignore the original memory >>> not owned by us and instead keep pointing at the allocated memory", >>>

Re: [PATCH V2 3/3] strbuf: allow to use preallocated memory

Jeff King writes: >> I think that call should reset line.buf to the original buffer on >> the stack, instead of saying "Ok, I'll ignore the original memory >> not owned by us and instead keep pointing at the allocated memory", >> as the allocation was done as a fallback measure. >

Re: [PATCH V2 3/3] strbuf: allow to use preallocated memory

On Mon, Jun 06, 2016 at 03:44:07PM -0700, Junio C Hamano wrote: > William Duclot writes: > > > I'm not sure to follow you. I agree that the "fixed strbuf" feature is > > flawed by the presence of this `die()`. But (unless misunderstanding) > > the

Re: [PATCH V2 3/3] strbuf: allow to use preallocated memory

William Duclot writes: > I'm not sure to follow you. I agree that the "fixed strbuf" feature is > flawed by the presence of this `die()`. But (unless misunderstanding) > the "owns_memory" bit you talk about does exist in this patch, and allow > the exact

Re: [PATCH V2 3/3] strbuf: allow to use preallocated memory

On Mon, Jun 06, 2016 at 10:19:07AM -0700, Junio C Hamano wrote: > William Duclot writes: > >> +#define MAX_ALLOC(a, b) (((a)>(b))?(a):(b)) > > I do not see why this macro is called MAX_ALLOC(); is there anything > "alloc" specific to what this does? You

Re: [PATCH V2 3/3] strbuf: allow to use preallocated memory

William Duclot writes: > +#define MAX_ALLOC(a, b) (((a)>(b))?(a):(b)) I do not see why this macro is called MAX_ALLOC(); is there anything "alloc" specific to what this does? You may happen to use it only for "alloc" related things, but that is not a

Re: [PATCH V2 3/3] strbuf: allow to use preallocated memory

I'm waiting for the discussion "is this useful" to settle before I do a final review, but I went quickly through the code and it seems OK. Just to show I read till the end: William Duclot writes: > +test_expect_success 'check preallocated strbuf behavior