Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add utstring_release #95

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Add utstring_release #95

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
78888bb
Add utstring_release
tbeu Nov 25, 2016
a4f1104
Add utstring_adopt and utstring_disown macros
tbeu Nov 30, 2016
7006e53
Fix utstring_disown
tbeu Dec 1, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions src/utstring.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,15 @@ do { \

#define utstring_body(s) ((s)->d)

static char* utstring_release(UT_string *s) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this isn't going to be a macro, I think it needs the word _UNUSED_ in front of it.
No comment on the functionality; I'll let @troydhanson take it or leave it.

Copy link
Owner

@troydhanson troydhanson Nov 27, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have done this kind of thing before manually. So, I think it's a useful operation. Agreed on the need for _UNUSED_. I think the name _release is so-so. I want to imply that it's releasing it "to" the caller. Maybe utstring_disown. Thoughts? Incidentally I once or twice wrote the opposite function that adopts the string you give it and makes a utstring around it. From that point of view, utstring_own and utstring_disown would make a pair.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Surely the opposite of utstring_disown would be utstring_adopt!

The longer I look at the existing utstring interface, the more I think it'll never quite line up with my C++ish expectations. Seems like I get a utstring by calling utstring_new(p), but then to put a string into it I have to use utstring_bincpy(p, "mydata", strlen("mydata")) (there's no utstring_strcpy), and then when I'm done I do utstring_free(p); p = NULL; (if I forget that reset-to-null operation, I'll get a double-free if I utstring_renew(p) — I think utstring_renew is an unusual wart in the API).

With this patch, if I want to "release/disown" the string, I do either char *s = utstring_body(p); utstring_init(p); or else char *s = utstring_release(p); p = NULL; (again forgetting the reset-to-null might lead to double-frees) — and if I want to "release/disown" a non-alloc'ed utstring, I have to use the first method, because utstring_release always wants to free the utstring I give it.

I'd want to separate the act of "disowning the buffer" and the act of "destroying the (possibly non-alloc'ed) utstring object itself":

_UNUSED_ static char *utstring_disown(UT_string *s) {
  char *ret = s->d;
  s->n = 0; s->i = 0; s->d = NULL;
  return ret;
}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@troydhanson wrote:

my intuition was that the original way (release does both) made sense

I can compellingly argue that in the C++ worldview they'd definitely be separate. But I can't argue that utstring currently follows the C++ worldview, because of the "renew" verb and the lack of (s)=NULLs in some places and the incomplete(?) support for UT_strings allocated on the stack. So if utstring isn't C++ish, there's no syllogistic reason for it to have to make disown and free separate operations.

The only thing I can say for sure is that disown/free should be separated if-and-only-if adopt/new are separated; i.e. if it's possible for an existing and/or stack-allocated utstring to adopt a new buffer, then symmetrically we must make it possible for a utstring to disown its buffer without vanishing and/or having been heap-allocated. Whereas, if utstring_adopt involves malloc(sizeof (UT_string)), then utstring_disown must involve free(s). Does that makes sense to everyone involved?

char* ret = NULL;
if (s != NULL) {
ret = s->d;
free(s);
}
return ret;
}

_UNUSED_ static void utstring_printf_va(UT_string *s, const char *fmt, va_list ap) {
int n;
va_list cp;
Expand Down