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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxx>
> ---
>  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.

> +                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...

> +                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...

> +            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 :))

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...

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
>  
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel




[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]     [Monitors]