Re: [Qemu-devel] [PATCH v2 11/14] qlit: improve QLit dict vs qdict comparison

["Marc-André Lureau" <marcandre.lureau@redhat.com>]

Hi

----- Original Message -----
> Marc-André Lureau <marcandre.lureau@redhat.com> writes:
> 
> > Verify that all qdict values are covered.
> >
> > (a previous iteration of this patch created a copy of qdict to remove
> > all checked elements, verifying no duplicates exist in the literal
> > qdict, however this is a programming error, and a size comparison
> > should be enough)
> 
> The parenthesis confused me, because I wasn't sure about the exact
> meaning of "a previous iteration of this patch".  I tried to find a
> prior commit, but it looks like you're really talking about v1.
> Confused me, because I don't expect commit messages discussing previous
> iterations.
> 
> What about:
> 
>     qlit: Tighten QLit dict vs qdict comparison
> 
>     We check that all members of the QLit dictionary are also in the
>     QDict.  We neglect to check the other direction.
> 
>     Comparing the number of members suffices, because QDict can't
>     contain duplicate members, and putting duplicates in a QLit is a
>     programming error.

Looks good to me

> 
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > ---
> >  qobject/qlit.c     | 37 +++++++++++++++++++++++--------------
> >  tests/check-qlit.c |  7 +++++++
> >  2 files changed, 30 insertions(+), 14 deletions(-)
> >
> > diff --git a/qobject/qlit.c b/qobject/qlit.c
> > index b1d9146220..dc0015f76c 100644
> > --- a/qobject/qlit.c
> > +++ b/qobject/qlit.c
> > @@ -41,6 +41,27 @@ static void compare_helper(QObject *obj, void *opaque)
> >          qlit_equal_qobject(&helper->objs[helper->index++], obj);
> >  }
> >  
> > +static bool qlit_equal_qdict(const QLitObject *lhs, const QDict *qdict)
> > +{
> > +    int i;
> > +
> > +    for (i = 0; lhs->value.qdict[i].key; i++) {
> > +        QObject *obj = qdict_get(qdict, lhs->value.qdict[i].key);
> > +
> > +        if (!qlit_equal_qobject(&lhs->value.qdict[i].value, obj)) {
> > +            return false;
> > +        }
> > +    }
> > +
> > +    /* Note: the literal qdict must not contain duplicates, this is
> > +     * considered a programming error and it isn't checked here. */
> > +    if (qdict_size(qdict) != i) {
> > +        return false;
> > +    }
> > +
> > +    return true;
> > +}
> > +
> >  bool qlit_equal_qobject(const QLitObject *lhs, const QObject *rhs)
> >  {
> >      if (!rhs || lhs->type != qobject_type(rhs)) {
> > @@ -55,20 +76,8 @@ bool qlit_equal_qobject(const QLitObject *lhs, const
> > QObject *rhs)
> >      case QTYPE_QSTRING:
> >          return (strcmp(lhs->value.qstr,
> >                         qstring_get_str(qobject_to_qstring(rhs))) == 0);
> > -    case QTYPE_QDICT: {
> > -        int i;
> > -
> > -        for (i = 0; lhs->value.qdict[i].key; i++) {
> > -            QObject *obj = qdict_get(qobject_to_qdict(rhs),
> > -                                     lhs->value.qdict[i].key);
> > -
> > -            if (!qlit_equal_qobject(&lhs->value.qdict[i].value, obj)) {
> > -                return false;
> > -            }
> > -        }
> > -
> > -        return true;
> > -    }
> > +    case QTYPE_QDICT:
> > +        return qlit_equal_qdict(lhs, qobject_to_qdict(rhs));
> >      case QTYPE_QLIST: {
> >          QListCompareHelper helper;
> >  
> > diff --git a/tests/check-qlit.c b/tests/check-qlit.c
> > index 47fde92a46..5d9558dfd9 100644
> > --- a/tests/check-qlit.c
> > +++ b/tests/check-qlit.c
> > @@ -28,6 +28,11 @@ static QLitObject qlit = QLIT_QDICT(((QLitDictEntry[]) {
> >      { },
> >  }));
> >  
> > +static QLitObject qlit_foo = QLIT_QDICT(((QLitDictEntry[]) {
> > +    { "foo", QLIT_QNUM(42) },
> > +    { },
> > +}));
> > +
> >  static QObject *make_qobject(void)
> >  {
> >      QDict *qdict = qdict_new();
> > @@ -51,6 +56,8 @@ static void qlit_equal_qobject_test(void)
> >  
> >      g_assert(qlit_equal_qobject(&qlit, qobj));
> >  
> > +    g_assert(!qlit_equal_qobject(&qlit_foo, qobj));
> > +
> >      qobject_decref(qobj);
> >  }
> 
> Ah, you do have negative test cases, just not in the patch creating the
> test.
> 
> When you leave gaps in that will be filled in later patches, I recomend
> pointing out the gaps in the commit message, with a promise they'll be
> filled shortly.  Your reviewers will thank you.
> 
> With am improved commit message,
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
> 

Thanks