2016-04-13 14:30 GMT+03:00 Pekka Paalanen <ppaala...@gmail.com>:
> On Tue, 12 Apr 2016 21:34:28 -0700
> Bryce Harrington <br...@osg.samsung.com> wrote:
>
>> On Wed, Apr 06, 2016 at 11:37:57AM +0300, Pekka Paalanen wrote:
>> > On Wed, 9 Mar 2016 16:49:29 -0800
>> > Bryce Harrington <br...@osg.samsung.com> wrote:
>> >
>> > > From: Giulio Camuffo <giuliocamu...@gmail.com>
>> > >
>> > > Signed-off-by: Bryce Harrington <br...@osg.samsung.com>
>> > > Reviewed-by: Quentin Glidic <sardemff7+...@sardemff7.net>
>> > > Acked-by: Pekka Paalanen <pekka.paala...@collabora.co.uk>
>> > > ---
>> > > v4: Update to current trunk
>> > > - Add missing param doc for mode in drm_output_choose_initial_mode
>> > > - Rebase to account for code changes by 91880f1e to make vt
>> > > switching configurable.
>> > >
>> > > Makefile.am | 3 +
>> > > src/compositor-drm.c | 196
>> > > ++++++++++++++++++---------------------------------
>> > > src/compositor.h | 2 -
>> > > src/main.c | 94 +++++++++++++++++++++++-
>> > > 4 files changed, 165 insertions(+), 130 deletions(-)
>> >
>> > Hi Giulio and Bryce,
>> >
>> > I'm sorry it has taken so long for me to come back to this.
>
>> > > +static int
>> > > +load_drm_backend(struct weston_compositor *c, const char *backend,
>> > > + int *argc, char **argv, struct weston_config *wc)
>> > > +{
>> > > + struct drm_config *config;
>> > > + struct weston_config_section *section;
>> > > + int ret = 0;
>> > > +
>> > > + config = zalloc(sizeof *config);
>> > > + if (!config)
>> > > + return -1;
>> >
>> > This function will be affected by the struct versioning too.
>>
>> The subsequent patch adds the versioning, although later in the routine
>> than here.
>>
>> > > + const struct weston_option options[] = {
>> > > + { WESTON_OPTION_INTEGER, "connector", 0,
>> > > &config->base.connector },
>> > > + { WESTON_OPTION_STRING, "seat", 0, &config->base.seat_id },
>> > > + { WESTON_OPTION_INTEGER, "tty", 0, &config->base.tty },
>> > > + { WESTON_OPTION_BOOLEAN, "current-mode", 0,
>> > > + &config->use_current_mode },
>> > > + { WESTON_OPTION_BOOLEAN, "use-pixman", 0,
>> > > &config->base.use_pixman },
>> > > + };
>> >
>> > Mixed declarations and code.
>>
>> Hmm, I'm not sure best how to address this. Options is being
>> initialized with pointers that are created by the zalloc, so I don't
>> think I can just separate the declaration from the initialization. Does
>> options need to be changed to be dynamically allocated as well?
>
> Hi,
>
> I suppose the simplest solution is to make another function, define
> 'options[]' in it, and get 'config' as an argument. Then it can call
> the parser and return. Something like ...fill_from_command_line(struct
> drm_config *config, argc, argv...).
>
> It's fine to keep your changes as separate patches as long as every
> patch is bisectable.
>
> I'm going through the new series right now, and noticed these emails
> from you just now.
I think this is one of the cases where strict style compliance goes in
the way of better code. Adding a function just adds an indirect level
but gives nothing in return, because it would really be just a wapper
around the options[] variable. I don't see it much different to a "int
value_1() { return 1; }".
But more importantly imho, it requires another revision.
Cheers,
Giulio
>
>
> Thanks,
> pq
_______________________________________________
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel
