Hi
----- Original Message -----
> Marc-André Lureau <marcandre.lur...@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.lur...@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 <arm...@redhat.com>
>
Thanks
