[Spice-devel] [PATCH spice-common v2] codegen: Remove duplicate client and server code from ChannelType::resolve

[Lukáš Hrázký]

On Thu, 2018-05-17 at 08:40 -0400, Frediano Ziglio wrote:
> > 
> > On Thu, 2018-05-17 at 07:36 -0400, Frediano Ziglio wrote:
> > > > 
> > > > On Wed, 2018-05-16 at 16:35 +0100, Frediano Ziglio wrote:
> > > > > Code that handled client and server messages check was the same, just
> > > > > changed some variable names.
> > > > > Instead use a class to store same information and reuse the code.
> > > > > This allows easier extension of the 2 path of code.
> > > > > 
> > > > > Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> > > > > ---
> > > > >  python_modules/ptypes.py | 68 +++++++++++++++++-----------------------
> > > > >  1 file changed, 29 insertions(+), 39 deletions(-)
> > > > > 
> > > > > Changes since v1:
> > > > > - avoid __dict__ usage.
> > > > > 
> > > > > diff --git a/python_modules/ptypes.py b/python_modules/ptypes.py
> > > > > index 0f6d8d6..fd9b1d0 100644
> > > > > --- a/python_modules/ptypes.py
> > > > > +++ b/python_modules/ptypes.py
> > > > > @@ -1021,56 +1021,46 @@ class ChannelType(Type):
> > > > >          return self.server_messages_byname[name]
> > > > >  
> > > > >      def resolve(self):
> > > > > -        if self.base != None:
> > > > > +        class MessagesInfo:
> > > > > +            def __init__(self, is_server, messages = [],
> > > > > messages_byname =
> > > > > {}):
> > > > 
> > > > PEP8 says no spaces around '=' in function aruments.
> > > > 
> > > 
> > > I'll do
> > > 
> > > > > +                self.is_server = is_server
> > > > > +                self.messages = messages[:]
> > > > 
> > > > If the purpose of [:] is to make a copy, I'd vouch for calling .copy()
> > > > to make it more explicit...
> > > > 
> > > 
> > > Does not work on Python 2, only Python 3, see:
> > > 
> > > > > > x=[]
> > > > > > x.copy()
> > > 
> > > Traceback (most recent call last):
> > >   File "<stdin>", line 1, in <module>
> > > AttributeError: 'list' object has no attribute 'copy'
> > 
> > Oh, ok, thought it'd be there.
> > 
> > > > > +                self.messages_byname = messages_byname.copy()
> > > > > +                self.count = 1
> > > > > +
> > > > > +        if self.base == None:
> > > > 
> > > > Strictly speaking, you should be using "self.base is None" instead of
> > > > the == operator...
> > > > 
> > > 
> > > I'll do it, previous code was doing a != so I kept same syntax
> > > but "is None" is surely more readable.
> > > 
> > > > > +            server_info = MessagesInfo(True)
> > > > > +            client_info = MessagesInfo(False)
> > > > > +        else:
> > > > >              self.base = self.base.resolve()
> > > > >  
> > > > > -            server_messages = self.base.server_messages[:]
> > > > > -            server_messages_byname =
> > > > > self.base.server_messages_byname.copy()
> > > > > -            client_messages = self.base.client_messages[:]
> > > > > -            client_messages_byname =
> > > > > self.base.client_messages_byname.copy()
> > > > > +            server_info = MessagesInfo(True,
> > > > > self.base.server_messages,
> > > > > +
> > > > > self.base.server_messages_byname)
> > > > > +            client_info = MessagesInfo(False,
> > > > > self.base.client_messages,
> > > > > +
> > > > > self.base.client_messages_byname)
> > > > 
> > > > It would probably be cleaner to copy the MessagesInfo class by either
> > > > implementing a MessagesInfo.copy() method that copies the whole object
> > > > or by using the copy module:
> > > > 
> > > > import copy
> > > > 
> > > > copied_info = copy.deepcopy(info_to_copy)
> > > > 
> > > > (at least I think this should work :))
> > > > 
> > > 
> > > I don't want to do a deep copy. Why I should?
> > 
> > If you called copy on the MessageInfo object, you'd need a deep copy.
> > 
> 
> no, I don't need it.
> 
> > > > You'd also have to make MessagesInfo object a member of this class,
> > > > instead of pulling the list and dict out of it at the end of the
> > > > patch...
> > > > 
> > > 
> > > What do you mean? Are you asking me to refactor all code?
> > 
> > I don't know what "all code" would mean here. As noted below, it was
> > only a suggestion...
> > 
> 
> Can you send a patch/code? I really don't get the suggestion.
> Note that MessagesInfo will have more information than what's
> stored in the class.

I just wanted to make a quick remark. It's not important, never mind.

> > > > Just a suggestion though, I might have missed something.
> > > > 
> > > > Cheers,
> > > > Lukas
> > > > 
> > > > >              # Set default member_name, FooChannel -> foo
> > > > >              self.member_name = self.name[:-7].lower()
> > > > > -        else:
> > > > > -            server_messages = []
> > > > > -            server_messages_byname = {}
> > > > > -            client_messages = []
> > > > > -            client_messages_byname = {}
> > > > >  
> > > > > -        server_count = 1
> > > > > -        client_count = 1
> > > > > -
> > > > > -        server = True
> > > > > +        info = server_info
> > > > >          for m in self.members:
> > > > >              if m == "server":
> > > > > -                server = True
> > > > > +                info = server_info
> > > > >              elif m == "client":
> > > > > -                server = False
> > > > > -            elif server:
> > > > > -                m.is_server = True
> > > > > -                m = m.resolve(self)
> > > > > -                if m.value:
> > > > > -                    server_count = m.value + 1
> > > > > -                else:
> > > > > -                    m.value = server_count
> > > > > -                    server_count = server_count + 1
> > > > > -                server_messages.append(m)
> > > > > -                server_messages_byname[m.name] = m
> > > > > +                info = client_info
> > > > >              else:
> > > > > -                m.is_server = False
> > > > > +                m.is_server = info.is_server
> > > > >                  m = m.resolve(self)
> > > > > -                if m.value:
> > > > > -                    client_count = m.value + 1
> > > > > -                else:
> > > > > -                    m.value = client_count
> > > > > -                    client_count = client_count + 1
> > > > > -                client_messages.append(m)
> > > > > -                client_messages_byname[m.name] = m
> > > > > -
> > > > > -        self.server_messages = server_messages
> > > > > -        self.server_messages_byname = server_messages_byname
> > > > > -        self.client_messages = client_messages
> > > > > -        self.client_messages_byname = client_messages_byname
> > > > > +                if not m.value:
> > > > > +                    m.value = info.count
> > > > > +                info.count = m.value + 1
> > > > > +                info.messages.append(m)
> > > > > +                info.messages_byname[m.name] = m
> > > > > +
> > > > > +        self.server_messages = server_info.messages
> > > > > +        self.server_messages_byname = server_info.messages_byname
> > > > > +        self.client_messages = client_info.messages
> > > > > +        self.client_messages_byname = client_info.messages_byname
> > > > >  
> > > > >          return self
> > > > >  
> > > 
> > > Frediano