Opened 3 years ago

Closed 3 years ago

#22342 closed defect (implemented)

Add a nice append-only stringbuffer, and refactor code to use it

Reported by: nickm Owned by: nickm
Priority: Medium Milestone: Tor: 0.3.3.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: review-group-24
Cc: Actual Points: .2
Parent ID: Points:
Reviewer: isis Sponsor:


Right now we have 2.5 strategies for building strings out of pieces:

1a) Sometimes, we use a stack-allocated buffer, and a pointer into that buffer, and we write into that buffer using tor_snprintf and strlcpy and friends.

1b) Sometimes we do the same with a heap-allocated buffer.

2) Sometimes we allocate a smartlist full of little strings, and use smartlist_join_strings() to turn it into one big string.

Both of these methods are cumbersome and at least somewhat inefficient. It would be better to have something that managed the buffer size, and supported commands like "extend with snprintf" or "extend with string".

Calling this "intro" because it doesn't require extensive knowledge of Tor; but it isn't a small task to do it right. This is something where we'd want to optimize for efficiency... though in the short run, we might just do it as a wrapper around smartlist_t and smartlist_join_strings().

Child Tickets

#23149closednickmRefactor buffer.c: split and rename functions.Core Tor/Tor

Change History (10)

comment:1 Changed 3 years ago by nickm

Keywords: intro removed
Milestone: Tor: unspecifiedTor: 0.3.2.x-final
Owner: set to nickm
Status: newaccepted

Calling the initial implementation for 0.3.2.

comment:2 Changed 3 years ago by nickm

initial work in a branch strbuf. will some back to this later.

comment:3 Changed 3 years ago by nickm

Wait, that branch is silly.

We already have a nice way to grow strings: buf_t. I'm going to refactor it in #23149 to make the API nicer, then add helper functions in strbuf_v2 on top of that.

comment:4 Changed 3 years ago by nickm

Milestone: Tor: 0.3.2.x-finalTor: 0.3.3.x-final
Status: acceptedneeds_review

My branch buf_for_stringbuilder here outlines what I'd like to do for this kind of code in the future, and replaces a couple of old janky functions to use this new interface.

I think we shouldn't look at it till 0.3.3. Maybe it isn't even a good idea.

comment:5 Changed 3 years ago by nickm

Actual Points: .2

comment:6 Changed 3 years ago by nickm

Keywords: review-group-24 added

review-group-24 is now open.

comment:7 Changed 3 years ago by isis

Reviewer: isis

comment:8 Changed 3 years ago by isis

Status: needs_reviewmerge_ready

In commit 68a90d09 you've got a docstring where you're doing the thing where you start a sentence and then I'm not sure what hap

/** Add a nul-terminated <b>string</b> to <b>buf</b>, but */

Other than that, everything looks great! The new API looks much cleaner, especially in the directory code. :)

comment:9 Changed 3 years ago by arma

+/** Return a heap-allocated string containing the contents of <b>buf</b>
+ * contents.

might want one fewer word too.

Overall it looks like a fine useful feature. I did not hunt in detail for bugs though. :)

comment:10 Changed 3 years ago by nickm

Resolution: implemented
Status: merge_readyclosed

Fixups added as buf_for_stringbuffer; squashed as buf_for_stringbuffer_squashed; merging to master!

Note: See TracTickets for help on using tickets.