> On Tue, 2016-10-11 at 09:55 -0400, Frediano Ziglio wrote: > > > > > > > > diff --git a/server/cursor-channel.h b/server/cursor-channel.h > > > > > index a3ddaa3..8b3bc17 100644 > > > > > --- a/server/cursor-channel.h > > > > > +++ b/server/cursor-channel.h > > > > > @@ -19,6 +19,17 @@ > > > > > # define CURSOR_CHANNEL_H_ > > > > > > > > > > #include "common-graphics-channel.h" > > > > > +#include "red-parse-qxl.h" > > > > > + > > > > > +G_BEGIN_DECLS > > > > > + > > > > > +#define TYPE_CURSOR_CHANNEL cursor_channel_get_type() > > > > > + > > > > > +#define CURSOR_CHANNEL(obj) (G_TYPE_CHECK_INSTANCE_CAST((obj), > > > > > TYPE_CURSOR_CHANNEL, CursorChannel)) > > > > > +#define CURSOR_CHANNEL_CLASS(klass) > > > > > (G_TYPE_CHECK_CLASS_CAST((klass), > > > > > TYPE_CURSOR_CHANNEL, CursorChannelClass)) > > > > > +#define IS_CURSOR_CHANNEL(obj) > > > > > (G_TYPE_CHECK_INSTANCE_TYPE((obj), > > > > > TYPE_CURSOR_CHANNEL)) > > > > > +#define IS_CURSOR_CHANNEL_CLASS(klass) > > > > > (G_TYPE_CHECK_CLASS_TYPE((klass), > > > > > TYPE_CURSOR_CHANNEL)) > > > > > +#define CURSOR_CHANNEL_GET_CLASS(obj) > > > > > (G_TYPE_INSTANCE_GET_CLASS((obj), > > > > > TYPE_CURSOR_CHANNEL, CursorChannelClass)) > > > > > > > > > > /** > > > > > * This type it's a RedChannel class which implement cursor > > > > > (mouse) > > > > > @@ -26,6 +37,22 @@ > > > > > * A pointer to CursorChannel can be converted to a > > > > > RedChannel. > > > > > */ > > > > > typedef struct CursorChannel CursorChannel; > > > > > +typedef struct CursorChannelClass CursorChannelClass; > > > > > +typedef struct CursorChannelPrivate CursorChannelPrivate; > > > > > + > > > > > +struct CursorChannel > > > > > +{ > > > > > + CommonGraphicsChannel parent; > > > > > + > > > > > + CursorChannelPrivate *priv; > > > > > +}; > > > > > + > > > > > +struct CursorChannelClass > > > > > +{ > > > > > + CommonGraphicsChannelClass parent_class; > > > > > +}; > > > > > + > > > > > +GType cursor_channel_get_type(void) G_GNUC_CONST; > > > > > > > > > > > > > Why we need to expose this here ? On the C file is not enough ? > > > > > > This is used above in all of the 'standard' GObject macros (e.g. > > > CURSOR_CHANNEL(), IS_CURSOR_CHANNEL(), etc). It's standard practice > > > to > > > expose it in the header. In this case, it's possible that we could > > > move > > > it down to the .c file, depending on whether any other files use > > > these > > > macros, but I don't see much advantage to doing so. I'd prefer to > > > leave > > > the standard GObject boilerplate here. > > > > > > > Some months ago my sister decided it was time to change the car. > > As she has some child she decided for a bigger car. She was also > > thinking > > about having additional space to allow to go to holidays. After > > viewing > > different car we discussed if it was worth buying a bigger car > > considering > > it was more expensive to buy and to maintain (insurance and fuel). > > At the end she decided to buy a car enough for the child and family > > but that was not worth buying some more big one just for the holiday > > opting for a car roof bag. > > The 'standard' solution for the car is buying a bigger car but not > > all standard solutions are the best. > > The main reason why the standard is that way is that it supports more > > cases and so it's easier to put in a guide or tutorial or a book... > > just that we are not writing a book or following a tutorial or guide. > > > > Exposing CursorChannel and CursorChannelClass in the header is a way > > to tell that this is the object structure and can be used for > > instance > > for inheritance. Some languages introduced the "final" keyword to > > avoid > > inheritance... C just do not support objects so not exposing the > > structure it's a way to use. > > Also is there is no need to expose the structure why to do it? > > Another reason in favor of not exposing it is that you could avoid to > > pay the penalty (memory, code and cpu) of the "priv" field. > > > > About all that macros boilerplate I don't understand why they > > don't came to a better solution, something like > > > > G_TYPE_CAST(pointer, RedChannel) > > > > instead of having to define all these macro for every object... > > These are for convenience. You could use G_TYPE_CHECK_INSTANCE_CAST() > directly if you want, but it's nicer to use a convenience macro like > CURSOR_CHANNEL(). > Yes, this is mainly the only used beside some exceptions. > > Require that if there is a GObject typename "RedChannel" you > > have a RedChannel_get_type() function does not seem so hard > > or cause so many problems (but probably I miss something). > > Also instead of > > > > if (IS_CURSOR_CHANNEL(obj)) > > > > why not using > > > > if (CURSOR_CHANNEL(obj)) > > > > and avoid to define 2 macros? > > It's mostly a matter of convention. Most people simply expect these > macros to exist with a standard naming convention for any GObject type. > It's true that not all of these macros are always used for every type. > For instance, In this particular case we don't use them except within > the cursor-channel.c implementation. So we could certainly move them > down to the .c file (or remove them entirely) as you suggested. But if > I came across a GObject implementation that didn't define the standard > cast macros in the header, it would strike me as odd. > > > > As macro only users of these headers can use them and we > > are the only users. > > So can't we define our macros so we don't have all that > > boilerplate? > > We could define our own macros, I suppose. But why? Hundreds of GObject > applications and libraries do it this way. I don't think there's much > to be gained by bucking tradition here and implementing our own custom > cast macros just to avoid a symbol in the header. > I'm more concerned about copy&paste and edit every time. I tried a different approach and I came with this patch diff --git a/server/red-channel-client.h b/server/red-channel-client.h index 3a09510..9d65595 100644 --- a/server/red-channel-client.h +++ b/server/red-channel-client.h @@ -38,27 +38,28 @@ G_BEGIN_DECLS #define RED_TYPE_CHANNEL_CLIENT red_channel_client_get_type() -#define RED_CHANNEL_CLIENT(obj) \ - (G_TYPE_CHECK_INSTANCE_CAST((obj), RED_TYPE_CHANNEL_CLIENT, RedChannelClient)) -#define RED_CHANNEL_CLIENT_CLASS(klass) \ - (G_TYPE_CHECK_CLASS_CAST((klass), RED_TYPE_CHANNEL_CLIENT, RedChannelClientClass)) -#define RED_IS_CHANNEL_CLIENT(obj) \ - (G_TYPE_CHECK_INSTANCE_TYPE((obj), RED_TYPE_CHANNEL_CLIENT)) -#define RED_IS_CHANNEL_CLIENT_CLASS(klass) \ - (G_TYPE_CHECK_CLASS_TYPE((klass), RED_TYPE_CHANNEL_CLIENT)) -#define RED_CHANNEL_CLIENT_GET_CLASS(obj) \ - (G_TYPE_INSTANCE_GET_CLASS((obj), RED_TYPE_CHANNEL_CLIENT, RedChannelClientClass)) +#define GOBJECT_DECLARE_TYPE(type, prefix, macro) \ + typedef struct type type; \ + typedef struct type ## Class type ## Class; \ + typedef struct type ## Private type ## Private; \ + GType prefix ## _get_type(void) G_GNUC_CONST; \ + static inline type *macro(void *obj) \ + { return G_TYPE_CHECK_INSTANCE_CAST(obj, prefix ## _get_type(), type); } \ + static inline type ## Class *macro ## _CLASS(void *klass) \ + { return G_TYPE_CHECK_CLASS_CAST(klass, prefix ## _get_type(), type ## Class); } \ + static inline gboolean IS_ ## macro(void *obj) \ + { return G_TYPE_CHECK_INSTANCE_TYPE(obj, prefix ## _get_type()); } \ + static inline gboolean IS_ ## macro ## _CLASS(void *klass) \ + { return G_TYPE_CHECK_CLASS_TYPE((klass), prefix ## _get_type()); } \ + static inline type ## Class *macro ## _GET_CLASS(void *obj) \ + { return G_TYPE_INSTANCE_GET_CLASS(obj, prefix ## _get_type(), type ## Class); } + +GOBJECT_DECLARE_TYPE(RedChannelClient, red_channel_client, RED_CHANNEL_CLIENT); +typedef struct RedChannel RedChannel; typedef struct RedClient RedClient; typedef struct IncomingHandler IncomingHandler; -typedef struct RedChannel RedChannel; -typedef struct RedChannelClient RedChannelClient; -typedef struct RedChannelClientClass RedChannelClientClass; -typedef struct RedChannelClientPrivate RedChannelClientPrivate; - -GType red_channel_client_get_type(void) G_GNUC_CONST; - /* * When an error occurs over a channel, we treat it as a warning * for spice-server and shutdown the channel. obviously the big macro should be defined in a separate header and reused. It basically defines the mainly boilerplate using 1 line instead of 10. I think the main reason for not using a technique like this in GObject directly is portability. This require a compiler supporting "static inline" which is C99. Looks like should not a problem in 2016 but I really had complaints (this year) when I started using some C99 feature on a portable project. But we rely on many feature that clang/gcc provides and these languages supported "static inline" much earlier then C99 (and we also use "static inline" already). > > > > > > > > > > > diff --git a/server/inputs-channel.c b/server/inputs-channel.c > > > > > index 83c1360..c351dad 100644 > > > > > --- a/server/inputs-channel.c > > > > > +++ b/server/inputs-channel.c > > > > > @@ -57,6 +57,83 @@ > > > > > #define RECEIVE_BUF_SIZE \ > > > > > (4096 + (REDS_AGENT_WINDOW_SIZE + > > > > > REDS_NUM_INTERNAL_AGENT_MESSAGES) * > > > > > SPICE_AGENT_MAX_DATA_SIZE) > > > > > > > > > > +G_DEFINE_TYPE(InputsChannel, inputs_channel, RED_TYPE_CHANNEL) > > > > > + > > > > > +#define CHANNEL_PRIVATE(o) (G_TYPE_INSTANCE_GET_PRIVATE((o), > > > > > TYPE_INPUTS_CHANNEL, InputsChannelPrivate)) > > > > > + > > > > > +struct InputsChannelPrivate > > > > > +{ > > > > > + uint8_t recv_buf[RECEIVE_BUF_SIZE]; > > > > > + VDAgentMouseState mouse_state; > > > > > + int src_during_migrate; > > > > > + SpiceTimer *key_modifiers_timer; > > > > > + > > > > > + SpiceKbdInstance *keyboard; > > > > > + SpiceMouseInstance *mouse; > > > > > + SpiceTabletInstance *tablet; > > > > > +}; > > > > > + > > > > > + > > > > > +static void > > > > > +inputs_channel_get_property(GObject *object, > > > > > + guint property_id, > > > > > + GValue *value, > > > > > + GParamSpec *pspec) > > > > > +{ > > > > > + switch (property_id) > > > > > + { > > > > > + default: > > > > > + G_OBJECT_WARN_INVALID_PROPERTY_ID(object, > > > > > property_id, > > > > > pspec); > > > > > + } > > > > > +} > > > > > + > > > > > +static void > > > > > +inputs_channel_set_property(GObject *object, > > > > > + guint property_id, > > > > > + const GValue *value, > > > > > + GParamSpec *pspec) > > > > > +{ > > > > > + switch (property_id) > > > > > + { > > > > > + default: > > > > > + G_OBJECT_WARN_INVALID_PROPERTY_ID(object, > > > > > property_id, > > > > > pspec); > > > > > + } > > > > > +} > > > > > + > > > > > > > > I noted there are these empty function here and in different > > > > file. > > > > Don't GObject provide some default functions like these? > > > > > > In general, GObject is a pain and has a lot of boilerplate. So I > > > often > > > use a GObject "generator" application that creates a bunch of > > > function > > > skeletons. In this case I simply forgot to delete the ones I didn't > > > need. > > > > > > > I personally don't like that much generators if they generate too > > much code as you'll have to maintain lot of code. > > In this case for instance if the next version of GObject decide to > > change the way to implement properties you'll have to update all > > that code. > > Just to make clear I don't consider programs like bison/flex/gperf > > as generators as you don't expect to maintain their output but > > you just use them in the chain. > > In case it wasn't clear, I was trying to suggest that I will remove > this extra code. I simply missed it. > > Jonathon > Frediano _______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel