You are viewing a plain text version of this content. The canonical link for it is here.
Posted to proton@qpid.apache.org by Michael Goulish <mg...@redhat.com> on 2013/02/09 12:34:34 UTC
messenger accessors
Most of the Messenger interface is accessor functions -- 53% --
19 functions out of a total of 39 .
16 of those have both 'get' and 'set', while 3 are get-only.
Those 3 do not have the word 'get' in their names, so you have
to look at code to realize that they are accessors like the others.
Could we consider single accessors for everything ?
This would require that all enumtypes use 0 to mean "not a value".
( It has always seemed to me that this is a good policy anyway.
You always need that non-value sooner or later. )
This change would reduce the size of the messenger interface by a
large fraction -- 9 fns, which seems like a good thing,
and would reduce the fraction of simple accessors to one-third of
the interface. It would also unify the naming convention.
( Accessors that must be get-only would simply not take an input,
as now. )
typedef enum {
PN_ACCEPT_MODE_NONE,
PN_ACCEPT_MODE_AUTO,
PN_ACCEPT_MODE_MANUAL
} pn_accept_mode_t;
pn_accept_mode_t
pn_messenger_get_accept_mode ( pn_messenger_t * messenger,
pn_accept_mode_t mode
)
{
if ( mode )
messenger->accept_mode = mode;
return messenger->accept_mode;
}
Re: messenger accessors
Posted by Rafael Schloming <rh...@alum.mit.edu>.
I don't think this will work in all cases, e.g. get/set_incoming_window is
a normal int, not a typedef, and in fact set_incoming_window returns an
error code if the value passed in is invalid. With the signature you're
suggesting there would be no way for a setter to indicate an error.
--Rafael
On Mon, Feb 11, 2013 at 9:14 AM, Michael Goulish <mg...@redhat.com>wrote:
>
> Sorry, I only thought of pointers and enumtypes.
>
> To take care of integer variables that allow zero, we could use a pattern
> like this:
>
> ( Would this work better for swig? )
>
>
>
> pn_whatever_t
> pn_messenger_whatever ( pn_messenger_t * messenger,
> pn_whatever_t whatever
> )
> {
> if ( ! PN_WHATEVER_NON_VALUE(whatever) )
> messenger->whatever = whatever;
>
> return messenger->whatever;
> }
>
>
> Then you get to define non value explicitly for each type.
>
> The only place this won't work, then is i.e. a float value that allows the
> whole numberline, but I bet we won't have any of those.
>
>
>
>
>
>
>
> ----- Original Message -----
> From: "Rafael Schloming" <rh...@alum.mit.edu>
> To: proton@qpid.apache.org
> Sent: Monday, February 11, 2013 8:50:54 AM
> Subject: Re: messenger accessors
>
> Unless I'm missing something, I don't think this pattern would work in the
> general case. For example it's valid to set a timeout of zero, so you'd
> either be stuck with no way to get a timeout without setting it, or you'd
> have no way to set it back to zero once you'd set it to something else.
>
> There's also some benefit to keeping the accessor pattern simple when it
> comes to using swig to produce bindings.
>
> --Rafael
>
> On Sat, Feb 9, 2013 at 6:34 AM, Michael Goulish <mg...@redhat.com>
> wrote:
>
> >
> > Most of the Messenger interface is accessor functions -- 53% --
> > 19 functions out of a total of 39 .
> >
> >
> > 16 of those have both 'get' and 'set', while 3 are get-only.
> > Those 3 do not have the word 'get' in their names, so you have
> > to look at code to realize that they are accessors like the others.
> >
> >
> > Could we consider single accessors for everything ?
> > This would require that all enumtypes use 0 to mean "not a value".
> > ( It has always seemed to me that this is a good policy anyway.
> > You always need that non-value sooner or later. )
> >
> >
> > This change would reduce the size of the messenger interface by a
> > large fraction -- 9 fns, which seems like a good thing,
> > and would reduce the fraction of simple accessors to one-third of
> > the interface. It would also unify the naming convention.
> > ( Accessors that must be get-only would simply not take an input,
> > as now. )
> >
> >
> >
> >
> > typedef enum {
> > PN_ACCEPT_MODE_NONE,
> > PN_ACCEPT_MODE_AUTO,
> > PN_ACCEPT_MODE_MANUAL
> > } pn_accept_mode_t;
> >
> >
> >
> >
> > pn_accept_mode_t
> > pn_messenger_get_accept_mode ( pn_messenger_t * messenger,
> > pn_accept_mode_t mode
> > )
> > {
> > if ( mode )
> > messenger->accept_mode = mode;
> >
> > return messenger->accept_mode;
> > }
> >
> >
> >
> >
> >
> >
> >
>
Re: messenger accessors
Posted by Michael Goulish <mg...@redhat.com>.
Sorry, I only thought of pointers and enumtypes.
To take care of integer variables that allow zero, we could use a pattern like this:
( Would this work better for swig? )
pn_whatever_t
pn_messenger_whatever ( pn_messenger_t * messenger,
pn_whatever_t whatever
)
{
if ( ! PN_WHATEVER_NON_VALUE(whatever) )
messenger->whatever = whatever;
return messenger->whatever;
}
Then you get to define non value explicitly for each type.
The only place this won't work, then is i.e. a float value that allows the whole numberline, but I bet we won't have any of those.
----- Original Message -----
From: "Rafael Schloming" <rh...@alum.mit.edu>
To: proton@qpid.apache.org
Sent: Monday, February 11, 2013 8:50:54 AM
Subject: Re: messenger accessors
Unless I'm missing something, I don't think this pattern would work in the
general case. For example it's valid to set a timeout of zero, so you'd
either be stuck with no way to get a timeout without setting it, or you'd
have no way to set it back to zero once you'd set it to something else.
There's also some benefit to keeping the accessor pattern simple when it
comes to using swig to produce bindings.
--Rafael
On Sat, Feb 9, 2013 at 6:34 AM, Michael Goulish <mg...@redhat.com> wrote:
>
> Most of the Messenger interface is accessor functions -- 53% --
> 19 functions out of a total of 39 .
>
>
> 16 of those have both 'get' and 'set', while 3 are get-only.
> Those 3 do not have the word 'get' in their names, so you have
> to look at code to realize that they are accessors like the others.
>
>
> Could we consider single accessors for everything ?
> This would require that all enumtypes use 0 to mean "not a value".
> ( It has always seemed to me that this is a good policy anyway.
> You always need that non-value sooner or later. )
>
>
> This change would reduce the size of the messenger interface by a
> large fraction -- 9 fns, which seems like a good thing,
> and would reduce the fraction of simple accessors to one-third of
> the interface. It would also unify the naming convention.
> ( Accessors that must be get-only would simply not take an input,
> as now. )
>
>
>
>
> typedef enum {
> PN_ACCEPT_MODE_NONE,
> PN_ACCEPT_MODE_AUTO,
> PN_ACCEPT_MODE_MANUAL
> } pn_accept_mode_t;
>
>
>
>
> pn_accept_mode_t
> pn_messenger_get_accept_mode ( pn_messenger_t * messenger,
> pn_accept_mode_t mode
> )
> {
> if ( mode )
> messenger->accept_mode = mode;
>
> return messenger->accept_mode;
> }
>
>
>
>
>
>
>
Re: messenger accessors
Posted by Rafael Schloming <rh...@alum.mit.edu>.
Unless I'm missing something, I don't think this pattern would work in the
general case. For example it's valid to set a timeout of zero, so you'd
either be stuck with no way to get a timeout without setting it, or you'd
have no way to set it back to zero once you'd set it to something else.
There's also some benefit to keeping the accessor pattern simple when it
comes to using swig to produce bindings.
--Rafael
On Sat, Feb 9, 2013 at 6:34 AM, Michael Goulish <mg...@redhat.com> wrote:
>
> Most of the Messenger interface is accessor functions -- 53% --
> 19 functions out of a total of 39 .
>
>
> 16 of those have both 'get' and 'set', while 3 are get-only.
> Those 3 do not have the word 'get' in their names, so you have
> to look at code to realize that they are accessors like the others.
>
>
> Could we consider single accessors for everything ?
> This would require that all enumtypes use 0 to mean "not a value".
> ( It has always seemed to me that this is a good policy anyway.
> You always need that non-value sooner or later. )
>
>
> This change would reduce the size of the messenger interface by a
> large fraction -- 9 fns, which seems like a good thing,
> and would reduce the fraction of simple accessors to one-third of
> the interface. It would also unify the naming convention.
> ( Accessors that must be get-only would simply not take an input,
> as now. )
>
>
>
>
> typedef enum {
> PN_ACCEPT_MODE_NONE,
> PN_ACCEPT_MODE_AUTO,
> PN_ACCEPT_MODE_MANUAL
> } pn_accept_mode_t;
>
>
>
>
> pn_accept_mode_t
> pn_messenger_get_accept_mode ( pn_messenger_t * messenger,
> pn_accept_mode_t mode
> )
> {
> if ( mode )
> messenger->accept_mode = mode;
>
> return messenger->accept_mode;
> }
>
>
>
>
>
>
>