| Message ID | 20201211171152.146877-19-armbru@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | Immutable QString, and also one JSON writer less | expand |
On 11/12/20 18:11, Markus Armbruster wrote: > QString supports modifying its string, but it's quite limited: you can > only append. The remaining callers use it for building an initial > string, never for modifying it later. > > Change keyval_parse_one() to do build the initial string with GString. > This is another step towards making QString immutable. > > Signed-off-by: Markus Armbruster <armbru@redhat.com> It's a bit unfortunate that the infamous "keyval: accept escaped commas in implied option" patch was already getting rid of mutable QString. It used a completely different mechanism, namely unescaping the string in place. This means that my patch was doing n+1 allocations, versus a best case of n and a generic case of O(n) for this patch. The difference does not really matter, though I still like my code better. Paolo > --- > util/keyval.c | 11 ++++++----- > 1 file changed, 6 insertions(+), 5 deletions(-) > > diff --git a/util/keyval.c b/util/keyval.c > index 7f625ad33c..be34928813 100644 > --- a/util/keyval.c > +++ b/util/keyval.c > @@ -189,7 +189,7 @@ static const char *keyval_parse_one(QDict *qdict, const char *params, > QDict *cur; > int ret; > QObject *next; > - QString *val; > + GString *val; > > key = params; > val_end = NULL; > @@ -263,7 +263,7 @@ static const char *keyval_parse_one(QDict *qdict, const char *params, > > if (key == implied_key) { > assert(!*s); > - val = qstring_from_substr(params, 0, val_end - params); > + val = g_string_new_len(params, val_end - params); > s = val_end; > if (*s == ',') { > s++; > @@ -276,7 +276,7 @@ static const char *keyval_parse_one(QDict *qdict, const char *params, > } > s++; > > - val = qstring_new(); > + val = g_string_new(NULL); > for (;;) { > if (!*s) { > break; > @@ -286,11 +286,12 @@ static const char *keyval_parse_one(QDict *qdict, const char *params, > break; > } > } > - qstring_append_chr(val, *s++); > + g_string_append_c(val, *s++); > } > } > > - if (!keyval_parse_put(cur, key_in_cur, val, key, key_end, errp)) { > + if (!keyval_parse_put(cur, key_in_cur, qstring_from_gstring(val), > + key, key_end, errp)) { > return NULL; > } > return s; >
Paolo Bonzini <pbonzini@redhat.com> writes: > On 11/12/20 18:11, Markus Armbruster wrote: >> QString supports modifying its string, but it's quite limited: you can >> only append. The remaining callers use it for building an initial >> string, never for modifying it later. >> Change keyval_parse_one() to do build the initial string with >> GString. >> This is another step towards making QString immutable. >> Signed-off-by: Markus Armbruster <armbru@redhat.com> > > It's a bit unfortunate that the infamous "keyval: accept escaped > commas in implied option" patch was already getting rid of mutable > QString. > > It used a completely different mechanism, namely unescaping the string > in place. This means that my patch was doing n+1 allocations, versus > a best case of n and a generic case of O(n) for this patch. The > difference does not really matter, though I still like my code better. My patch is not intended as a replacement of yours. Mine does much less. I had to choose between creating a conflict and holding back my series while we figure out what to do with your patch. The dilemma is my own doing; your patch is waiting just for me. I picked the conflict. I can look into rebasing your patch on top of mine.
On 11/01/21 14:05, Markus Armbruster wrote: > I had to choose between creating a conflict and holding back my series > while we figure out what to do with your patch. The dilemma is my own > doing; your patch is waiting just for me. I picked the conflict. > > I can look into rebasing your patch on top of mine. No need to. My patch also removes the use of GString, so the code after my patch can be exactly the same. Or in other words, squashing a revert in front of my patch just works. Paolo
diff --git a/util/keyval.c b/util/keyval.c index 7f625ad33c..be34928813 100644 --- a/util/keyval.c +++ b/util/keyval.c @@ -189,7 +189,7 @@ static const char *keyval_parse_one(QDict *qdict, const char *params, QDict *cur; int ret; QObject *next; - QString *val; + GString *val; key = params; val_end = NULL; @@ -263,7 +263,7 @@ static const char *keyval_parse_one(QDict *qdict, const char *params, if (key == implied_key) { assert(!*s); - val = qstring_from_substr(params, 0, val_end - params); + val = g_string_new_len(params, val_end - params); s = val_end; if (*s == ',') { s++; @@ -276,7 +276,7 @@ static const char *keyval_parse_one(QDict *qdict, const char *params, } s++; - val = qstring_new(); + val = g_string_new(NULL); for (;;) { if (!*s) { break; @@ -286,11 +286,12 @@ static const char *keyval_parse_one(QDict *qdict, const char *params, break; } } - qstring_append_chr(val, *s++); + g_string_append_c(val, *s++); } } - if (!keyval_parse_put(cur, key_in_cur, val, key, key_end, errp)) { + if (!keyval_parse_put(cur, key_in_cur, qstring_from_gstring(val), + key, key_end, errp)) { return NULL; } return s;
QString supports modifying its string, but it's quite limited: you can only append. The remaining callers use it for building an initial string, never for modifying it later. Change keyval_parse_one() to do build the initial string with GString. This is another step towards making QString immutable. Signed-off-by: Markus Armbruster <armbru@redhat.com> --- util/keyval.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-)