[PATCH] shell: fix set_transient

[Pekka Paalanen]

On Thu, 8 Dec 2011 13:05:42 -0500
Kristian Høgsberg <krh at bitplanet.net> wrote:

> On Thu, Dec 8, 2011 at 10:08 AM, Pekka Paalanen <ppaalanen at gmail.com>
> wrote:
> > On Thu, 08 Dec 2011 17:04:47 +0200
> > Tiago Vignatti <tiago.vignatti at linux.intel.com> wrote:
...
> >> but I thought you'd implement the explicit cast scheme.. meh.
> >
> > Sorry, not today :-D
> >
> > I'll keep it in mind, though, for when I have time for it.
> >
> > For reference to others: it is for improving type safety in the
> > compositor side, by putting some real pointer types into struct
> > wl_*_interface's function arguments.
> 
> The question there is, what types?  We could make a struct
> wl_compositor_resource that wraps the generic wl_resource, and then
> move the void *data out of wl_resource and into wl_compsitor_resource
> as a typesafe pointer.  To what though?  We could make it struct
> wl_compositor * and just require that the compositor use those types.
> I dunno, it just seems messy and I'm not sure it's worth it.

Yes, it's more difficult than it first appeared, I thought about it a
bit more on my way home yesterday. My idea was something like this:

If there is a callback parameter that has the type of wl_shell_surface
in the protocol, we could make the argument type in the C function
signature as (struct wl_shell_surface *). The client side already uses
these types as opaque pointers.

The trick is, we cannot pass the (struct wl_resource *) directly there,
we need to pass wl_resource->data, cast into (struct wl_shell_surface
*).

This has the following implications:
- We always have to use the wl_surface::data member to point to
  something that allows to access the underlying struct wl_resource.
  Accessing the wl_resource becomes a little more verbose.

- callback boilerplate would be:

static void
shell_set_foo(struct wl_client *client,
	      struct wl_shell_surface *shell_surface)
{
	struct shell_surface *shsurf =
		(struct shell_surface *)shell_surface;

	...
}

which shows the benefit: we still have a cast, but the cast is
explicit. The problem I am trying to solve, is that wl_resource::data
is a void* which can be implicitly cast to whatever.

We could take the type safety one step further, and define private
helpers in e.g. shell.c in this case:

static inline struct shell_surface *
to_shell_surface(struct wl_shell_surface *p)
{
	return (struct shell_surface *)p;
}

That would allow explicit casting from a single type to a single type,
the compiler giving out warnings about type mismatches on all other
cases:

- if the protocol changes and you forget to change your callback
  function signature (because the function signatures in wl_*_interface
  structs are autogenerated from the protocol and no longer match)

- you change the function signature, but forget to change the cast
  (because the inline helper function types no longer match)

I have not tried to actually implement this yet, so there's probably
more stuff I have overlooked.

Do you think this idea would be good to try?


- pq