[PATCH wayland] scanner: server-side also emit SINCE macros for requests

[Jonas Ådahl]

On Wed, Oct 14, 2015 at 12:36:46PM +0300, Pekka Paalanen wrote:
> On Wed, 14 Oct 2015 17:05:43 +0800
> Jonas Ådahl <jadahl at gmail.com> wrote:
> 
> > On Wed, Oct 14, 2015 at 11:52:36AM +0300, Pekka Paalanen wrote:
> > > On Wed, 14 Oct 2015 10:32:36 +0300
> > > Giulio Camuffo <giuliocamuffo at gmail.com> wrote:
> > > 
> > > > 2015-10-14 4:06 GMT+03:00 Jonas Ådahl <jadahl at gmail.com>:
> > > > > On Tue, Oct 13, 2015 at 04:18:19PM -0700, Bryce Harrington wrote:
> > > > >> On Fri, Oct 09, 2015 at 10:07:48PM +0200, David FORT wrote:
> > > > >> > Server-side we need to know the versions for destructor requests.
> > > > >> >
> > > > >> > Signed-off-by: David FORT <contact at hardening-consulting.com>
> > > > >>
> > > > >> Reviewed-by: Bryce Harrington <bryce at osg.samsung.com>
> > > > >>
> > > > >> > ---
> > > > >> >  src/scanner.c | 1 +
> > > > >> >  1 file changed, 1 insertion(+)
> > > > >> >
> > > > >> > diff --git a/src/scanner.c b/src/scanner.c
> > > > >> > index f456aa5..406ba82 100644
> > > > >> > --- a/src/scanner.c
> > > > >> > +++ b/src/scanner.c
> > > > >> > @@ -1266,6 +1266,7 @@ emit_header(struct protocol *protocol, enum side side)
> > > > >> >                     emit_structs(&i->request_list, i, side);
> > > > >> >                     emit_opcodes(&i->event_list, i);
> > > > >> >                     emit_opcode_versions(&i->event_list, i);
> > > > >> > +                   emit_opcode_versions(&i->request_list, i);
> > > > >
> > > > > This will cause compilation error when an event and a request in the
> > > > > same interface has the same name. Do we have that restriction already?
> > > > 
> > > > That sounds real confusing. If we don't, we should have it, imho.
> > > 
> > > We kind of sort of may have it...
> > > 
> > > See this made up xml:
> > > 
> > > <protocol name="dupe">
> > > 
> > >   <interface name="stupid" version="2">
> > >     <request name="dupp" />
> > >     <event name="dummy">
> > >       <description summary="bump the opcode" />
> > >     </event>
> > >     <event name="dupp" since="2" />
> > >   </interface>
> > > 
> > > </protocol>
> > > 
> > > This will already conflict without any "since" #definitions if both
> > > client and server header are included in the same compilation unit,
> > > because server header has
> > > 
> > > #define STUPID_DUPP	1
> > > 
> > > and client header has
> > > 
> > > #define STUPID_DUPP	0
> > > 
> > > This is a far-fetched example, but I think it shows that we really
> > > should not have requests and events of the same name.
> > > 
> > > I would propose the following:
> > > 
> > > 1. Patch wayland-scanner to reject protocols where an interface has
> > >    both a request and an event with the same name.
> > > 2. Wait for a stable Wayland release.
> > > 3. Start relying on never to have a request and an event with the same name.
> > 
> > Do you mean we should revert the commit that causes the scanner to
> > generate
> > 
> > #define STUPID_DUPP	1
> > 
> > until after the next release? Or should we assume these hypothetical
> > protocols don't act as both clients and servers in the same file?o
> 
> We can't revert, it was already there since before 0.85. :-)
> This is the opcode #def, not the "since".

Ah, I missed that we are not talking about _SINCE any longer.

> 
> I mean we should hold off on this thread's patch before we are sure
> enough we don't get into trouble for it.
> 
> > Also, if we make a release where we reject such protocols, then we could
> > just as well start rely on it already (at build time). If it'd be just a
> > warning, we can wait with relying on it when we make that warning an
> > error.
> 
> It could be a warning, sure. But it doesn't matter if it's an error or
> a warning, if it gets hit by a stable protocol extension.
> 
> I'm only saying that it is much easier to revert the rejection, than to
> remove things from generated API, if it turns out we seriously broke
> something.
> 
> > Relying on it at runtime; I don't see in what scenario we would need to
> > care about this.
> 
> I never talked about runtime. If you apply this patch, you said we will
> rely on never having the same name in both requests and events (if they
> have different "since"). Otherwise the build will fail.
> 
> > > This is overly cautious, but I think it is possible people have got
> > > away with using a same name, and we should know if anyone wants to
> > > complain while we have a chance to back out.
> 

Do we have any use cases for the new _SINCE macros that this patch
introduces? If not, I guess we can just as well wait until after the
release without causing any inconveniences.


Jonas