[PATCH weston v3 00/17] compositor-wayland: refactor the configuration API

[Pekka Paalanen]

On Thu,  5 May 2016 22:45:38 +0200
Benoit Gschwind <gschwind at gnu-log.net> wrote:

> v3:
>  - rebase to master
>  - heavy patches spliting to help review as suggested by Pekka
>  - fix typo.
>  - rename functions and variables as suggested by Pekka
> 
> Benoit Gschwind (17):
>   compositor-wayland: fix misc. typo
>   compositor-wayland: add versionning to config structure
>   compositor-wayland: rename wayland_output_init_from_config
>   compositor-wayland: refactor wayland_output_config_init
>   compositor-wayland: move configuration parsing to main.c
>   compositor-wayland: refactor load_wayland_backend
>   compositor-wayland: refactor load_wayland_backend
>   compositor-wayland: refactor load_wayland_backend
>   compositor-wayland: refactor wayland_backend_config_add_new_output
>   compositor-wayland: refactor wayland_backend_config_release
>   compositor-wayland: refactor wayland_backend_create
>   compositor-wayland: refactor wayland_backend_create
>   compositor-wayland: refactor backend_init
>   compositor-wayland: refactor backend_init
>   compositor-wayland: rename wayland_backend_config_release
>   compositor-wayland: rename wayland_output_config_init
>   compositor-wayland: rename wayland_backend_config_add_new_output
> 
>  src/compositor-wayland.c | 237 +++++------------------------------------------
>  src/compositor-wayland.h |   3 +
>  src/main.c               | 223 +++++++++++++++++++++++++++++++++++++++++++-
>  3 files changed, 246 insertions(+), 217 deletions(-)
> 

Hi Benoit,

thanks for the splitting. It may seem much, but it does make reviewing
them very easy.

The first thing that catches my eye is that several patches have
identical summaries. That is a very poor practise, as the summary is
supposed to be practically a unique name for the patch. Also, if there
are three patches that all just "refactor load_wayland_backend", why
are they not squashed into one? If each patch does something different,
the summary should be different too. So, fix the messages, please.

Several patches got the same comment as patch 4 that it is an ok
change, but the commit message and summary needs improving. Patches
that I did not explictly reply to but get the same comments are: 10 -
17.

I also saw some repeated renames, like wayland_output_init_from_config
-> wayland_output_config_init -> weston_wayland_output_config_init. The
intermediate name has no benefit, so just rename once to the final
name, please.


Thanks,
pq
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 811 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20160506/d3e26f6b/attachment.sig>