[Spice-devel] Using private structure for making structure more public...

[Frediano Ziglio]

> 
> On Fri, May 13, 2016 at 04:42:08AM -0400, Frediano Ziglio wrote:
> > Hi,
> >   this is the last though looking at refactory branch current trend.
> > 
> > We use Gobject and private separation so structure are less exposed
> > to external... bla bla bla.
> > 
> > The implementation however looks to not agree with it!
> > 
> > Usually the channel and the client (like InputsChannel and
> > InputsChannelClient) are quite tight so each other know and access
> > their internal. This could lead to bad encapsulation. So current
> > status is:
> > - single file for channel/client (beside display which is
> >   huge and split in different files);
> > - access mostly free from one structure to another. Which is
> >   bad.
> > So the actual transformation is:
> > - split channel and client;
> > - add accessors to deal with encapsulation;
> > - move structures to headers;
> > - change structures to Gobjects
> > The result however from the external point of view is that
> > users of channels include these giant headers with a lot
> > of internal details while before in headers there were very
> > small information, mainly you have these structures (without
> > any types beside base class) with these methods.
> > 
> > The most visible example of this is CursorChannel(Client).
> > These have (currently master state) very limited header (one!)
> > while at the end it's quite huge.
> > 
> > Now... from the abstract point of view. Why a client of a channel
> > have to know details on the client of the same channel?
> > I think it's just a design mistake (at lest mostly).
> > 
> > Back to code it's quite a mistake that channel headers include
> > client ones, should not. Would be also better to have a kind of
> > external interface header and a specific channel <-> specific client
> > header to avoid exposing all accessors.
> 
> I haven't looked at the Channel/Client case in particular, but

but this is the "bla bla bla" I was stating above... the theory is X
so it's right.

I remember quite vaguely a physics law stating that no matter
which is the path the work was the same... well, try to do shopping
passing through Bejing and New York and back to your town and tell
me if it tooks the same amount of work.

> the general plan is indeed to go from one huge file with everyone
> accessing any structure to split files with structures in headers when
> needed (which is mostly accomplished by now).
> Then what we are trying to do now is to add some common
> structure/encapsulation to all of this, through some object model
> (gobject) and private data. The expectation is that it will remove more
> interdependencies between C files. Maybe this is not what is happening
> right now with Client/Channel, but the expectation is that with less
> poking at internal structures, it will be easier to do more decoupling,
> and to think about the overall design. So it's very likely/not
> unexpected that some design improvements can be found.
> If some patches are a big step backward from a design perspective, it's
> worth raising this during review, otherwise my hope is that even if
> things are not perfect, we are still making progress towards something
> saner.
> 
> Christophe
> 

I honestly think that the path is important as important is
not blindly change the code.

There are many reasons.
- stability. The last stable release was not based on master, right?
  This as the code confidence is less which could slow down future
  work/backport/fixes;
- readability of the code. The more patterns you put on the code the
  more hard it's to understand it. I'm talking about how to handle
  memory, encapsulation, threading or similar;
- effort. Let say that to fix the encapsulation we increase memory
  usage a lot. If this goes too over we'll have to work on memory
  usage before a future release.

Let me make some examples:
- looking at InputsChannel split most of the functions are poking on
  other structures using accessors here and there and to follow some
  paths you have continuously jump from a file to another making
  harder a future debugging of the code. I could agree that this is
  just a temporary step (and honestly this files aren't that big)
  but still it's a regression. Also if you look at the code some
  details could be easily moved in different way (for instance 
  PIPE enum can be moved to client header) to satisfy both the
  way we are actually moving and keep the headers smaller instead
  of going blindly and then having to fix the headers with more
  efforts;
- looking at the LZ4 patches nobody noted the different pattern on
  the same code handling memory?

One thing I suggested in the initial mail is to try some headers
inclusion change, I'm not changing everything.

To sum up "Please don't go too much out of tracks".

Oh... next stable release should IMO be based on master!

Frediano