Re: [PATCH v2 3/8] qnum: QNumValue type for QNum value literals

[Eduardo Habkost]

On Fri, Nov 20, 2020 at 06:29:16AM +0100, Markus Armbruster wrote:
> Eduardo Habkost <ehabkost@redhat.com> writes:
> 
> > On Thu, Nov 19, 2020 at 11:24:52AM +0100, Markus Armbruster wrote:
> >> Marc-André Lureau <marcandre.lureau@gmail.com> writes:
> >> 
> >> > On Tue, Nov 17, 2020 at 6:42 PM Eduardo Habkost <ehabkost@redhat.com> 
> >> > wrote:
> >> >
> >> >> On Tue, Nov 17, 2020 at 12:37:56PM +0400, Marc-André Lureau wrote:
> >> >> > On Tue, Nov 17, 2020 at 2:43 AM Eduardo Habkost <ehabkost@redhat.com>
> >> >> wrote:
> >> >> >
> >> >> > > Provide a separate QNumValue type that can be used for QNum value
> >> >> > > literals without the referencing counting and memory allocation
> >> >> > > features provided by QObject.
> >> >> > >
> >> >> > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> >> >> > > ---
> >> >> > > Changes v1 -> v2:
> >> >> > > * Fix "make check" failure, by updating check-qnum unit test to
> >> >> > >   use the new struct fields
> >> >> > > ---
> >> >> > >  include/qapi/qmp/qnum.h | 40 +++++++++++++++++++--
> >> >> > >  qobject/qnum.c          | 78 
> >> >> > > ++++++++++++++++++++---------------------
> >> >> > >  tests/check-qnum.c      | 14 ++++----
> >> >> > >  3 files changed, 84 insertions(+), 48 deletions(-)
> >> >> > >
> >> >> > > diff --git a/include/qapi/qmp/qnum.h b/include/qapi/qmp/qnum.h
> >> >> > > index 55c27b1c24..62fbdfda68 100644
> >> >> > > --- a/include/qapi/qmp/qnum.h
> >> >> > > +++ b/include/qapi/qmp/qnum.h
> >> >> > > @@ -46,20 +46,56 @@ typedef enum {
> >> >> > >   * in range: qnum_get_try_int() / qnum_get_try_uint() check range 
> >> >> > > and
> >> >> > >   * convert under the hood.
> >> >> > >   */
> >> >> > > -struct QNum {
> >> >> > > -    struct QObjectBase_ base;
> >> >> > > +
> >> >> > > +/**
> >> >> > > + * struct QNumValue: the value of a QNum
> >> >> > > + *
> >> >> > > + * QNumValue literals can be constructed using the `QNUM_VAL_INT`,
> >> >> > > + * `QNUM_VAL_UINT`, and `QNUM_VAL_DOUBLE` macros.
> >> >> > > + */
> >> >> > > +typedef struct QNumValue {
> >> >> > > +    /* private: */
> >> 
> >> Do we care?
> >
> > Are you asking if we want to make them private, or if we want to
> > document them as private (assuming they are).
> >
> > The answer to the latter is yes ("private:" is an indication to
> > kernel-doc).  The answer to the former seems to be "no", based on
> > your other comments.
> >
> > Or maybe `kind` should be public and `u` should be private?
> 
> You're factoring out a part of struct QNum into new struct QNumValue.
> struct QNum is not private before the patch.  I see no need to start
> making it or parts of it private now.

I don't want to change the rules, just to document the existing
implicit rules.  If you say QNum.u was never private, I won't
argue.

> 
> When the structure of a data type is to be kept away from its users, I
> prefer to keep it out of the public header, so the compiler enforces the
> encapsulation.

I prefer that too, except that it is impossible when users of the
API need the compiler to know the struct size.

-- 
Eduardo