On Wed, Feb 19, 2020 at 03:43:26AM +0200, Robear Selwans wrote: > On Tue, Feb 18, 2020 at 10:46 PM Junio C Hamano <gitster@xxxxxxxxx> wrote: > > Abhishek Kumar <abhishekkumar8222@xxxxxxxxx> writes: > > >> STRBUF_INIT_CONST: a new way to initialize strbuf > > > Use imperative mood and be more specific in the commit title - > > > `strbuf: Teach strbuf to initialize immutable strings` > > s/T/t/; > I don't get what you mean by that. We don't usually capitalize the sentence fragment in a subject line (hence "replace T with t", but said in sed jargon). > > Also, isn't "if (sb->alloc < sb->len)" too loose a check for the new > > feature? AFAICS in 1/2, a strbuf that is still borrowing a const > > string always has sb->alloc==0. Other instances of strbuf that > > happens to satisify the above condition, e.g. (sb->len == 5 && > > sb->alloc == 1), is an error. If we are to check the condition > > about sb->len, shouldn't we diagnose such a case as an error, no? > AFAIK after reading the documentation for `strbuf`, there is no other > case where `sb->len > sb->alloc` as `alloc` needs to always be more > than `len`. I'd like to be corrected if mistaken, though. Yeah, I don't see how it could be for an allocated buffer, since that would imply writing past the allocated size. > > As Peff, I am a bit hesitant about leaving a strbuf that hasn't been > > made mutable around, though. > Yeah, I started to get that when I read Peff's reply. I think I'll go with > the approach that Peff suggested, where the constant string is > copied to a stack array and so is made mutable to a degree. Since > this is a different way, I guess I'll make that from scratch instead of > editing the existing one. Yeah, I think it's sufficiently different to start from scratch. Do note that I think you may need to stuff an extra bit into the struct somehow. For a const string, you can set alloc to "0" to denote the situation. But if we're going to be able to attach another buffer that can be written to (but may not yet be full), you'd need to be able to set alloc to the true size of that buffer. And then we need some way to note that it's not a real heap buffer. There are probably clever tricks you could play with the low bits of the "alloc" field to avoid making the struct any bigger. But given the way we use strbufs, I suspect it may not bring a measurable performance improvement. And I do have dreams of eventually adding more bits. E.g., there are a few cases where it would be convenient to extend the "strbuf that points to a stack buffer but grows" into "strbuf that points to a stack buffer but truncates" (e.g., in our error handling routines which want to avoid calling malloc). That's definitely a few steps out from where we are now, of course, but we may get there eventually. -Peff
