| Message ID | 20101112134026.0d825aa4@doriath |
|---|---|
| State | New |
| Headers | show |
Luiz Capitulino <lcapitulino@redhat.com> writes: > On Fri, 12 Nov 2010 16:04:39 +0100 > Markus Armbruster <armbru@redhat.com> wrote: > >> Luiz Capitulino <lcapitulino@redhat.com> writes: >> >> > On Fri, 12 Nov 2010 15:16:33 +0100 >> > Markus Armbruster <armbru@redhat.com> wrote: >> > >> >> Luiz Capitulino <lcapitulino@redhat.com> writes: >> >> >> >> > On Fri, 12 Nov 2010 11:21:57 +0100 >> >> > Markus Armbruster <armbru@redhat.com> wrote: >> >> > >> >> >> Luiz Capitulino <lcapitulino@redhat.com> writes: >> >> [...] >> >> >> > +QString *qemu_chr_mem_to_qs(CharDriverState *chr) >> >> >> > +{ >> >> >> > + MemoryDriver *d = chr->opaque; >> >> >> > + >> >> >> > + if (d->outbuf_size == 0) { >> >> >> > + return qstring_new(); >> >> >> > + } >> >> >> >> >> >> Why is this necessary? Is qstring_from_substr() broken for empty >> >> >> substrings? If it is, it ought to be fixed! >> >> > >> >> > qstring_from_substr() takes a character range; outbuf_size stores a size, >> >> > not a string length. So we do: >> >> > >> >> >> > + return qstring_from_substr((char *) d->outbuf, 0, d->outbuf_size - 1); >> >> > >> >> > If outbuf_size is 0, we'll be passing a negative value down. >> >> >> >> What's wrong with that? >> > >> > Although it's going to work with the current QString implementation, I don't >> > think it's it's a good idea to rely on a negative index. >> >> How should I extract the substring of S beginning at index B with length >> L? If I cant't do this for any B, L with interval [B,B+L-1] fully >> within [0,length(S)], then the API is flawed, and ought to be replaced. > > Not sure we're talking about the same problem, anymore. When you said: > >> >> What's wrong with that? > > What did you mean? Did you mean 'let's not decrement outbuf_size' or did > you mean 'let's pass -1 anyway'? Yes, what's wrong with qstring_from_substr(S, 0, -1)? Its function comment is imprecise, it doesn't tell us whether the END-th character is included in the substring or not. The code, however, is clear enough: it *is* included. And the unit test checks that. Therefore, qstring_from_substr("abc", 0, 0) returns the qstring "a". > Both seem wrong to me: the substring [0,-1] should be invalid Why? How do you express "the empty substring starting at 0" then? > and not > decrementing outbuf_size is wrong, because it contains the buffer size and > qstring_from_substr() will consume an additional char from the buffer (which > should be '\0' today, but we shouldn't count on that). > >> >> > Maybe, we could have: >> > >> > return qstring_from_substr((char *) d->outbuf, 0, >> > d->outbuf_size > 0 ? d->outbuf_size - 1 : 0); >> > >> > A bit harder to read, but makes the function smaller. >> >> Err, doesn't qstring_from_substr(s, 0, 0) extract a substring of length >> 1? > > Yeah, it's a bug. But that doesn't change my suggestion, can we do this way? > > This should fix the bug (not even compiled tested): > > diff --git a/qstring.c b/qstring.c > index 4e2ba08..72a25de 100644 > --- a/qstring.c > +++ b/qstring.c > @@ -42,10 +42,10 @@ QString *qstring_from_substr(const char *str, int start, int end) > > qstring = qemu_malloc(sizeof(*qstring)); > > - qstring->length = end - start + 1; > - qstring->capacity = qstring->length; > + qstring->length = end - start; > + qstring->capacity = qstring->length + 1; > > - qstring->string = qemu_malloc(qstring->capacity + 1); > + qstring->string = qemu_malloc(qstring->capacity); > memcpy(qstring->string, str + start, qstring->length); > qstring->string[qstring->length] = 0; I suspect this will fail your unit test.
On Fri, 12 Nov 2010 17:06:16 +0100 Markus Armbruster <armbru@redhat.com> wrote: > Luiz Capitulino <lcapitulino@redhat.com> writes: > > > On Fri, 12 Nov 2010 16:04:39 +0100 > > Markus Armbruster <armbru@redhat.com> wrote: > > > >> Luiz Capitulino <lcapitulino@redhat.com> writes: > >> > >> > On Fri, 12 Nov 2010 15:16:33 +0100 > >> > Markus Armbruster <armbru@redhat.com> wrote: > >> > > >> >> Luiz Capitulino <lcapitulino@redhat.com> writes: > >> >> > >> >> > On Fri, 12 Nov 2010 11:21:57 +0100 > >> >> > Markus Armbruster <armbru@redhat.com> wrote: > >> >> > > >> >> >> Luiz Capitulino <lcapitulino@redhat.com> writes: > >> >> [...] > >> >> >> > +QString *qemu_chr_mem_to_qs(CharDriverState *chr) > >> >> >> > +{ > >> >> >> > + MemoryDriver *d = chr->opaque; > >> >> >> > + > >> >> >> > + if (d->outbuf_size == 0) { > >> >> >> > + return qstring_new(); > >> >> >> > + } > >> >> >> > >> >> >> Why is this necessary? Is qstring_from_substr() broken for empty > >> >> >> substrings? If it is, it ought to be fixed! > >> >> > > >> >> > qstring_from_substr() takes a character range; outbuf_size stores a size, > >> >> > not a string length. So we do: > >> >> > > >> >> >> > + return qstring_from_substr((char *) d->outbuf, 0, d->outbuf_size - 1); > >> >> > > >> >> > If outbuf_size is 0, we'll be passing a negative value down. > >> >> > >> >> What's wrong with that? > >> > > >> > Although it's going to work with the current QString implementation, I don't > >> > think it's it's a good idea to rely on a negative index. > >> > >> How should I extract the substring of S beginning at index B with length > >> L? If I cant't do this for any B, L with interval [B,B+L-1] fully > >> within [0,length(S)], then the API is flawed, and ought to be replaced. > > > > Not sure we're talking about the same problem, anymore. When you said: > > > >> >> What's wrong with that? > > > > What did you mean? Did you mean 'let's not decrement outbuf_size' or did > > you mean 'let's pass -1 anyway'? > > Yes, what's wrong with qstring_from_substr(S, 0, -1)? > > Its function comment is imprecise, it doesn't tell us whether the END-th > character is included in the substring or not. > > The code, however, is clear enough: it *is* included. And the unit test > checks that. > > Therefore, qstring_from_substr("abc", 0, 0) returns the qstring "a". > > > Both seem wrong to me: the substring [0,-1] should be invalid > > Why? > > How do you express "the empty substring starting at 0" then? I didn't consider that when I wrote the code, so it's a matter a defining the behavior we want it to have. > > > and not > > decrementing outbuf_size is wrong, because it contains the buffer size and > > qstring_from_substr() will consume an additional char from the buffer (which > > should be '\0' today, but we shouldn't count on that). > > > >> > >> > Maybe, we could have: > >> > > >> > return qstring_from_substr((char *) d->outbuf, 0, > >> > d->outbuf_size > 0 ? d->outbuf_size - 1 : 0); > >> > > >> > A bit harder to read, but makes the function smaller. > >> > >> Err, doesn't qstring_from_substr(s, 0, 0) extract a substring of length > >> 1? > > > > Yeah, it's a bug. But that doesn't change my suggestion, can we do this way? > > > > This should fix the bug (not even compiled tested): > > > > diff --git a/qstring.c b/qstring.c > > index 4e2ba08..72a25de 100644 > > --- a/qstring.c > > +++ b/qstring.c > > @@ -42,10 +42,10 @@ QString *qstring_from_substr(const char *str, int start, int end) > > > > qstring = qemu_malloc(sizeof(*qstring)); > > > > - qstring->length = end - start + 1; > > - qstring->capacity = qstring->length; > > + qstring->length = end - start; > > + qstring->capacity = qstring->length + 1; > > > > - qstring->string = qemu_malloc(qstring->capacity + 1); > > + qstring->string = qemu_malloc(qstring->capacity); > > memcpy(qstring->string, str + start, qstring->length); > > qstring->string[qstring->length] = 0; > > I suspect this will fail your unit test. Haven't checked it yet, but maybe it has to be fixed too.
diff --git a/qstring.c b/qstring.c index 4e2ba08..72a25de 100644 --- a/qstring.c +++ b/qstring.c @@ -42,10 +42,10 @@ QString *qstring_from_substr(const char *str, int start, int end) qstring = qemu_malloc(sizeof(*qstring)); - qstring->length = end - start + 1; - qstring->capacity = qstring->length; + qstring->length = end - start; + qstring->capacity = qstring->length + 1; - qstring->string = qemu_malloc(qstring->capacity + 1); + qstring->string = qemu_malloc(qstring->capacity); memcpy(qstring->string, str + start, qstring->length); qstring->string[qstring->length] = 0;