Using OutBuffer makes it easy to forget to zero-terminate strings and create dangling pointers. I'm think we should make OutBuffer a little less error prone.
Changes:
- Get rid of toChars
- Add extractString which checks for a '\0' then takes ownership of the internal buffer
- Add scopedString which checks for a '\0' then return a reference to the internal buffer
- Add scopedData which returns a reference to the internal buffer
Benefit:
- Nobody will use toChars thinking that it copies/owns the data
- Harder to forget to zero-terminate strings
- Hopefully less casting
Comment #1 by andrej.mitrovich — 2013-02-06T18:22:42Z
(In reply to comment #0)
> Using OutBuffer makes it easy to forget to zero-terminate strings and create
> dangling pointers. I'm think we should make OutBuffer a little less error
> prone.
>
> Changes:
> - Get rid of toChars
There's also toString, which seems to be almost the same as toChars. And the OutBuffer implementation is split across src/root/root.c and src/backend/outbuf.c
Comment #2 by andrej.mitrovich — 2013-02-06T18:29:02Z
So if I got it right we'd have:
char *OutBuffer::extractString()
{
writeByte(0);
char *p;
p = (char *)data;
data = NULL;
offset = 0;
size = 0;
return p;
}
char *OutBuffer::scopedString()
{
writeByte(0);
return (char *)data;
}
char *OutBuffer::scopedData()
{
return (char *)data;
}
Is that right?
Comment #3 by yebblies — 2013-02-06T19:55:05Z
(In reply to comment #2)
The functions shouldn't append a '\0' if data already ends with one, and I'm not sure scopedString should _ever_ modify it. Other than that it looks right.
Comment #4 by andrej.mitrovich — 2013-02-07T11:44:09Z
(In reply to comment #3)
> (In reply to comment #2)
>
> The functions shouldn't append a '\0' if data already ends with one, and I'm
> not sure scopedString should _ever_ modify it. Other than that it looks right.
Then I don't understand what you mean by: "Add scopedString which checks for a '\0' then return a reference to the internal buffer".
It checks for a \0, and does what if it doesn't find it?
Comment #5 by yebblies — 2013-02-07T16:43:19Z
(In reply to comment #4)
> (In reply to comment #3)
> > (In reply to comment #2)
> >
> > The functions shouldn't append a '\0' if data already ends with one, and I'm
> > not sure scopedString should _ever_ modify it. Other than that it looks right.
>
> Then I don't understand what you mean by: "Add scopedString which checks for a
> '\0' then return a reference to the internal buffer".
>
> It checks for a \0, and does what if it doesn't find it?
assert(0)
The idea being that getting a pointer to the internal string shouldn't change the contents. Maybe it would be more useful to append a zero, I don't know.
Comment #6 by andrej.mitrovich — 2013-02-12T20:30:07Z
(In reply to comment #5)
> The idea being that getting a pointer to the internal string shouldn't change
> the contents.
Ok, but then what do we need scopedData for? If we're returning a char* and it's not zero-terminated I don't see how it can be used.
So far I've got this:
/** Extract a zero-terminated string.
The caller takes ownership. */
const char *OutBuffer::extractString()
{
if (!data || data[offset] != '\0')
writeByte(0);
char *p;
p = (char *)data;
data = NULL;
offset = 0;
size = 0;
return p;
}
/** Verify the internal buffer is zero-terminated
and return a reference to it. */
const char *OutBuffer::scopedString()
{
assert(data && data[offset] == '\0');
return (char *)data;
}
Comment #7 by andrej.mitrovich — 2013-02-12T20:31:04Z
(In reply to comment #6)
> (In reply to comment #5)
> > The idea being that getting a pointer to the internal string shouldn't change
> > the contents.
>
> Ok, but then what do we need scopedData for? If we're returning a char* and
> it's not zero-terminated I don't see how it can be used.
>
> So far I've got this:
Those casts should have been (const char*).
Comment #8 by andrej.mitrovich — 2013-02-12T20:37:10Z
(In reply to comment #7)
> (In reply to comment #6)
> > (In reply to comment #5)
> > > The idea being that getting a pointer to the internal string shouldn't change
> > > the contents.
> >
> > Ok, but then what do we need scopedData for? If we're returning a char* and
> > it's not zero-terminated I don't see how it can be used.
> >
> > So far I've got this:
>
> Those casts should have been (const char*).
Or maybe they should remain (char *) if ownership is taken. :)
Comment #9 by yebblies — 2013-02-12T20:39:57Z
(In reply to comment #6)
> (In reply to comment #5)
> > The idea being that getting a pointer to the internal string shouldn't change
> > the contents.
>
> Ok, but then what do we need scopedData for? If we're returning a char* and
> it's not zero-terminated I don't see how it can be used.
>
You would use it in the same places as scopedString, but when using OutBuffer for binary instead of text. i.e. when you know the data won't be escaped.
Comment #10 by andrej.mitrovich — 2013-02-12T20:44:36Z
(In reply to comment #9)
> (In reply to comment #6)
> > (In reply to comment #5)
> > > The idea being that getting a pointer to the internal string shouldn't change
> > > the contents.
> >
> > Ok, but then what do we need scopedData for? If we're returning a char* and
> > it's not zero-terminated I don't see how it can be used.
> >
>
> You would use it in the same places as scopedString, but when using OutBuffer
> for binary instead of text. i.e. when you know the data won't be escaped.
Code in DMD already uses .data directly though.