archive mirror
 help / color / mirror / Atom feed
From: Jeff King <>
To: Robear Selwans <>
Cc: "Junio C Hamano" <>,
	"Abhishek Kumar" <>,, "René Scharfe" <>,
	"Nguyễn Thái Ngọc Duy" <>,
	"Pratik Karki" <>
Subject: Re: [GSoC][RFC][PATCH 2/2] STRBUF_INIT_CONST: Adapting strbuf_* functions
Date: Tue, 18 Feb 2020 21:05:50 -0500	[thread overview]
Message-ID: <> (raw)
In-Reply-To: <>

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 <> wrote:
> > Abhishek Kumar <> 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.


  reply	other threads:[~2020-02-19  2:05 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-02-18  9:30 [GSoC][RFC][PATCH 2/2] STRBUF_INIT_CONST: Adapting strbuf_* functions Abhishek Kumar
2020-02-18 14:42 ` Robear Selwans
2020-02-18 20:46 ` Junio C Hamano
2020-02-19  1:43   ` Robear Selwans
2020-02-19  2:05     ` Jeff King [this message]
2020-02-19  3:13     ` Junio C Hamano
2020-02-19  4:34       ` Robear Selwans
2020-02-19 10:44         ` Junio C Hamano
  -- strict thread matches above, loose matches on Subject: below --
2020-02-18  4:18 [GSoC][RFC][PATCH 0/2] STRBUF_INIT_CONST Cover Robear Selwans
2020-02-18  4:18 ` [GSoC][RFC][PATCH 2/2] STRBUF_INIT_CONST: Adapting strbuf_* functions Robear Selwans

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \ \ \ \ \ \ \ \ \ \

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).