On Thu, 14 Apr 2016 19:09:34 +0300 Giulio Camuffo <giuliocamu...@gmail.com> wrote:
> 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.
No, it would be much more. The function would essentially be "fill in
this config struct from these argc,argv". That color would actually
please my eye more, as that is a logical single step to execute from a
load_*() function, rather than stuffing it all inline in load_*().
But, I'm not the one building the shed here. If I really care, I can
fix it after landing this stuff, since it doesn't seem to break the
build.
Thanks,
pq
pgpBXXpQpCFKO.pgp
Description: OpenPGP digital signature
_______________________________________________ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
