Re: [PATCH 3/5] scanner: introduce --object-type option

[Pekka Paalanen]

On Fri, 26 Jan 2018 14:34:53 +
Emil Velikov  wrote:

> On 26 January 2018 at 02:44, Jonas Ådahl  wrote:
> > On Thu, Jan 25, 2018 at 05:56:45PM +, Emil Velikov wrote:  
> >> On 25 January 2018 at 02:01, Jonas Ådahl  wrote:  
> >> > On Wed, Jan 24, 2018 at 07:15:09PM +, Emil Velikov wrote:  
> >> >> On 24 January 2018 at 18:20, Derek Foreman  
> >> >> wrote:  
> >> >> > On 2018-01-22 09:30 AM, Emil Velikov wrote:  

> >> >> >> Considering the status of the the name conflict series, should I
> >> >> >> re-spin this lot?
> >> >> >> I'm more than happy to tweak things - say rename the toggle, etc.  
> >> >> >
> >> >> >
> >> >> > I see there were two series proposed to control symbol visibility, 
> >> >> > yours and
> >> >> > Jonas'?
> >> >> >
> >> >> > Assuming that once we drop the symbol collision issue they both solve 
> >> >> > the
> >> >> > same problems, it would be good if we could focus on one going 
> >> >> > forward.
> >> >> >
> >> >> > Is this the chosen one?
> >> >> >  
> >> >> Right, the cover letter [1] covers some of the high-lights/differences.
> >> >> As a TL;DR: using static/shared is more common and gives us more
> >> >> flexibility for the future.
> >> >>
> >> >> So far Pekka is the only person who commented on the series/approach
> >> >> and seemed happy.
> >> >> I was expecting others to weight in - say Jonas ;-) I'll respin the
> >> >> series tomorrow.
> >> >>
> >> >> In hindsight --object-type={shared,static} is too much of a mouthful,
> >> >> I'll opt for --{static,shared} for v2.  
> >> >
> >> > I have no opinion of what variant to land (I'm slightly too lazy to
> >> > search for my own, and this is high up the inbox thanks to the replies,
> >> > so lets focus on this one).
> >> >
> >> > My only nit is using the term "object-type". I think refering to it as
> >> > "visibility" ("symbol visibility" if wanting to be extra verbose) where
> >> > one can say 'export', 'static' or 'private' is more accurate.
> >> >
> >> > "objects" are a Wayland protocol thing and that is not what we are
> >> > poking at here.
> >> >  
> >> Right using "object" might be misleading. On the other hand,
> >> --shared/--static are well known and dead-obvious.
> >> Plus it gives us flexibility to sweep more things under it, if we get
> >> some funky stuff in the future.
> >>
> >> Can I buy you on the different name?  
> >
> > --static is pretty clear, but --shared? Both WL_EXPORT and WL_PRIVATE
> > are "shared" but just different audiences.
> >  
> The actual symbol notation WL_EXPORT/WL_PRIVATE/other(?) is an
> implementation detail.
> That is "hidden" within the C files and never exposed outside.
> 
> There are two reasons for that:
>  - used internally, thus no point in polluting the namespace
>  - some compilers (older clang or was it the oracle/sun one?) are
> unhappy if the header is annotates, while the C file isn't

Hi,

looks like I too want to bikeshed about the option names.

I suppose "static" refers to static linking, not the static keyword in
C language? Took me a moment to realize that, but if everyone else
thinks it's obvious, fine.

I have pretty much the same comment on "shared" as Jonas. I suppose
it's more clear if you see that the options are either shared or
static. Would there be any downsides to using "export"? After thinking
it about a bit, "shared" is fine by me too.

In the end, I believe this is just a question of the help text for
these switches, is it clear enough.

Btw. Emil, would it hurt to have the generated code to #define
WL_PRIVATE only if it is not already defined? That would leave the door
open to user project hacks to define it to "static" if they really need
to be able to use conflicting protocols. Otherwise they'd run the
generated code once more through sed or awk.


Thanks,
pq


pgpr2OKInwF02.pgp
Description: OpenPGP digital signature
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH 3/5] scanner: introduce --object-type option

[Emil Velikov]

On 26 January 2018 at 02:44, Jonas Ådahl  wrote:
> On Thu, Jan 25, 2018 at 05:56:45PM +, Emil Velikov wrote:
>> On 25 January 2018 at 02:01, Jonas Ådahl  wrote:
>> > On Wed, Jan 24, 2018 at 07:15:09PM +, Emil Velikov wrote:
>> >> On 24 January 2018 at 18:20, Derek Foreman  wrote:
>> >> > On 2018-01-22 09:30 AM, Emil Velikov wrote:
>> >> >>
>> >> >> On 22 August 2017 at 14:02, Emil Velikov  
>> >> >> wrote:
>> >> >>>
>> >> >>> On 18 August 2017 at 13:05, Pekka Paalanen  
>> >> >>> wrote:
>> >> >>>
>> >> >>
>> >> >> The exported configuration would then be:
>> >> >> LOCAL_INTERFACE_DECL=extern
>> >> >> EXTERN_INTERFACE_DECL=extern
>> >> >> LOCAL_INTERFACE_DEF=WL_EXPORT
>> >> >>
>> >> >> That would be far too flexible and no-one would use it right, 
>> >> >> right?
>> >> >>
>> >> > I did not introduce separate tokens, since those are (and should be)
>> >> > used _only_ in the .c file.
>> >> > Personally then do not seem necessary, If you prefer we can add them
>> >> > though.
>> >> 
>> >> 
>> >>  Ah, no, that was just a wild idea of something completely different. 
>> >>  I
>> >>  meant that the user project would be setting those macros before 
>> >>  using
>> >>  scanner-generated files, and if unset, the scanner-emitted code would
>> >>  default to the legacy behaviour. That way there would be no 
>> >>  visibility
>> >>  modes in scanner itself. If it's not obviously better, then 
>> >>  nevermind.
>> >>  It certainly has a lot more room to go wrong than your proposal.
>> >> 
>> >> 
>> >> >>> I see.
>> >> >>>
>> >> >>> Personally I'd lean towards with my approach for now since it is
>> >> >>> simpler, despite that it provides less flexibility.
>> >> >>> As you pointed out the proposal is a bit more fragile, so might be
>> >> >>> better to avoid until there's a real need for it.
>> >> >>>
>> >> >>>
>> >>  ...
>> >> 
>> >> >> The patch looks pretty much correct to me, if we choose to go this
>> >> >> way.
>> >> >>
>> >> > Glad to hear.
>> >> >
>> >> > I'll let me know once you guys are settled in on the approach, and
>> >> > I'll respin the series with all the comments addressed.
>> >> 
>> >> 
>> >>  Cool, let's see if we can get the name conflict issue solved, and 
>> >>  then
>> >>  I'll try to remember to ping you.
>> >> 
>> >> >>> Ack, I'll keep an eye open, just in case.
>> >> >>>
>> >> >> Considering the status of the the name conflict series, should I
>> >> >> re-spin this lot?
>> >> >> I'm more than happy to tweak things - say rename the toggle, etc.
>> >> >
>> >> >
>> >> > I see there were two series proposed to control symbol visibility, 
>> >> > yours and
>> >> > Jonas'?
>> >> >
>> >> > Assuming that once we drop the symbol collision issue they both solve 
>> >> > the
>> >> > same problems, it would be good if we could focus on one going forward.
>> >> >
>> >> > Is this the chosen one?
>> >> >
>> >> Right, the cover letter [1] covers some of the high-lights/differences.
>> >> As a TL;DR: using static/shared is more common and gives us more
>> >> flexibility for the future.
>> >>
>> >> So far Pekka is the only person who commented on the series/approach
>> >> and seemed happy.
>> >> I was expecting others to weight in - say Jonas ;-) I'll respin the
>> >> series tomorrow.
>> >>
>> >> In hindsight --object-type={shared,static} is too much of a mouthful,
>> >> I'll opt for --{static,shared} for v2.
>> >
>> > I have no opinion of what variant to land (I'm slightly too lazy to
>> > search for my own, and this is high up the inbox thanks to the replies,
>> > so lets focus on this one).
>> >
>> > My only nit is using the term "object-type". I think refering to it as
>> > "visibility" ("symbol visibility" if wanting to be extra verbose) where
>> > one can say 'export', 'static' or 'private' is more accurate.
>> >
>> > "objects" are a Wayland protocol thing and that is not what we are
>> > poking at here.
>> >
>> Right using "object" might be misleading. On the other hand,
>> --shared/--static are well known and dead-obvious.
>> Plus it gives us flexibility to sweep more things under it, if we get
>> some funky stuff in the future.
>>
>> Can I buy you on the different name?
>
> --static is pretty clear, but --shared? Both WL_EXPORT and WL_PRIVATE
> are "shared" but just different audiences.
>
The actual symbol notation WL_EXPORT/WL_PRIVATE/other(?) is an
implementation detail.
That is "hidden" within the C files and never exposed outside.

There are two reasons for that:
 - used internally, thus no point in polluting the namespace
 - some compilers (older clang or was it the oracle/sun one?) are
unhappy if the header is annotates, while the C file isn't

-Emil
___
wayland-devel 

Re: [PATCH 3/5] scanner: introduce --object-type option

[Jonas Ådahl]

On Thu, Jan 25, 2018 at 05:56:45PM +, Emil Velikov wrote:
> On 25 January 2018 at 02:01, Jonas Ådahl  wrote:
> > On Wed, Jan 24, 2018 at 07:15:09PM +, Emil Velikov wrote:
> >> On 24 January 2018 at 18:20, Derek Foreman  wrote:
> >> > On 2018-01-22 09:30 AM, Emil Velikov wrote:
> >> >>
> >> >> On 22 August 2017 at 14:02, Emil Velikov  
> >> >> wrote:
> >> >>>
> >> >>> On 18 August 2017 at 13:05, Pekka Paalanen  wrote:
> >> >>>
> >> >>
> >> >> The exported configuration would then be:
> >> >> LOCAL_INTERFACE_DECL=extern
> >> >> EXTERN_INTERFACE_DECL=extern
> >> >> LOCAL_INTERFACE_DEF=WL_EXPORT
> >> >>
> >> >> That would be far too flexible and no-one would use it right, right?
> >> >>
> >> > I did not introduce separate tokens, since those are (and should be)
> >> > used _only_ in the .c file.
> >> > Personally then do not seem necessary, If you prefer we can add them
> >> > though.
> >> 
> >> 
> >>  Ah, no, that was just a wild idea of something completely different. I
> >>  meant that the user project would be setting those macros before using
> >>  scanner-generated files, and if unset, the scanner-emitted code would
> >>  default to the legacy behaviour. That way there would be no visibility
> >>  modes in scanner itself. If it's not obviously better, then nevermind.
> >>  It certainly has a lot more room to go wrong than your proposal.
> >> 
> >> 
> >> >>> I see.
> >> >>>
> >> >>> Personally I'd lean towards with my approach for now since it is
> >> >>> simpler, despite that it provides less flexibility.
> >> >>> As you pointed out the proposal is a bit more fragile, so might be
> >> >>> better to avoid until there's a real need for it.
> >> >>>
> >> >>>
> >>  ...
> >> 
> >> >> The patch looks pretty much correct to me, if we choose to go this
> >> >> way.
> >> >>
> >> > Glad to hear.
> >> >
> >> > I'll let me know once you guys are settled in on the approach, and
> >> > I'll respin the series with all the comments addressed.
> >> 
> >> 
> >>  Cool, let's see if we can get the name conflict issue solved, and then
> >>  I'll try to remember to ping you.
> >> 
> >> >>> Ack, I'll keep an eye open, just in case.
> >> >>>
> >> >> Considering the status of the the name conflict series, should I
> >> >> re-spin this lot?
> >> >> I'm more than happy to tweak things - say rename the toggle, etc.
> >> >
> >> >
> >> > I see there were two series proposed to control symbol visibility, yours 
> >> > and
> >> > Jonas'?
> >> >
> >> > Assuming that once we drop the symbol collision issue they both solve the
> >> > same problems, it would be good if we could focus on one going forward.
> >> >
> >> > Is this the chosen one?
> >> >
> >> Right, the cover letter [1] covers some of the high-lights/differences.
> >> As a TL;DR: using static/shared is more common and gives us more
> >> flexibility for the future.
> >>
> >> So far Pekka is the only person who commented on the series/approach
> >> and seemed happy.
> >> I was expecting others to weight in - say Jonas ;-) I'll respin the
> >> series tomorrow.
> >>
> >> In hindsight --object-type={shared,static} is too much of a mouthful,
> >> I'll opt for --{static,shared} for v2.
> >
> > I have no opinion of what variant to land (I'm slightly too lazy to
> > search for my own, and this is high up the inbox thanks to the replies,
> > so lets focus on this one).
> >
> > My only nit is using the term "object-type". I think refering to it as
> > "visibility" ("symbol visibility" if wanting to be extra verbose) where
> > one can say 'export', 'static' or 'private' is more accurate.
> >
> > "objects" are a Wayland protocol thing and that is not what we are
> > poking at here.
> >
> Right using "object" might be misleading. On the other hand,
> --shared/--static are well known and dead-obvious.
> Plus it gives us flexibility to sweep more things under it, if we get
> some funky stuff in the future.
> 
> Can I buy you on the different name?

--static is pretty clear, but --shared? Both WL_EXPORT and WL_PRIVATE
are "shared" but just different audiences.


Jonas

> Emil
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH 3/5] scanner: introduce --object-type option

[Emil Velikov]

On 25 January 2018 at 02:01, Jonas Ådahl  wrote:
> On Wed, Jan 24, 2018 at 07:15:09PM +, Emil Velikov wrote:
>> On 24 January 2018 at 18:20, Derek Foreman  wrote:
>> > On 2018-01-22 09:30 AM, Emil Velikov wrote:
>> >>
>> >> On 22 August 2017 at 14:02, Emil Velikov  wrote:
>> >>>
>> >>> On 18 August 2017 at 13:05, Pekka Paalanen  wrote:
>> >>>
>> >>
>> >> The exported configuration would then be:
>> >> LOCAL_INTERFACE_DECL=extern
>> >> EXTERN_INTERFACE_DECL=extern
>> >> LOCAL_INTERFACE_DEF=WL_EXPORT
>> >>
>> >> That would be far too flexible and no-one would use it right, right?
>> >>
>> > I did not introduce separate tokens, since those are (and should be)
>> > used _only_ in the .c file.
>> > Personally then do not seem necessary, If you prefer we can add them
>> > though.
>> 
>> 
>>  Ah, no, that was just a wild idea of something completely different. I
>>  meant that the user project would be setting those macros before using
>>  scanner-generated files, and if unset, the scanner-emitted code would
>>  default to the legacy behaviour. That way there would be no visibility
>>  modes in scanner itself. If it's not obviously better, then nevermind.
>>  It certainly has a lot more room to go wrong than your proposal.
>> 
>> 
>> >>> I see.
>> >>>
>> >>> Personally I'd lean towards with my approach for now since it is
>> >>> simpler, despite that it provides less flexibility.
>> >>> As you pointed out the proposal is a bit more fragile, so might be
>> >>> better to avoid until there's a real need for it.
>> >>>
>> >>>
>>  ...
>> 
>> >> The patch looks pretty much correct to me, if we choose to go this
>> >> way.
>> >>
>> > Glad to hear.
>> >
>> > I'll let me know once you guys are settled in on the approach, and
>> > I'll respin the series with all the comments addressed.
>> 
>> 
>>  Cool, let's see if we can get the name conflict issue solved, and then
>>  I'll try to remember to ping you.
>> 
>> >>> Ack, I'll keep an eye open, just in case.
>> >>>
>> >> Considering the status of the the name conflict series, should I
>> >> re-spin this lot?
>> >> I'm more than happy to tweak things - say rename the toggle, etc.
>> >
>> >
>> > I see there were two series proposed to control symbol visibility, yours 
>> > and
>> > Jonas'?
>> >
>> > Assuming that once we drop the symbol collision issue they both solve the
>> > same problems, it would be good if we could focus on one going forward.
>> >
>> > Is this the chosen one?
>> >
>> Right, the cover letter [1] covers some of the high-lights/differences.
>> As a TL;DR: using static/shared is more common and gives us more
>> flexibility for the future.
>>
>> So far Pekka is the only person who commented on the series/approach
>> and seemed happy.
>> I was expecting others to weight in - say Jonas ;-) I'll respin the
>> series tomorrow.
>>
>> In hindsight --object-type={shared,static} is too much of a mouthful,
>> I'll opt for --{static,shared} for v2.
>
> I have no opinion of what variant to land (I'm slightly too lazy to
> search for my own, and this is high up the inbox thanks to the replies,
> so lets focus on this one).
>
> My only nit is using the term "object-type". I think refering to it as
> "visibility" ("symbol visibility" if wanting to be extra verbose) where
> one can say 'export', 'static' or 'private' is more accurate.
>
> "objects" are a Wayland protocol thing and that is not what we are
> poking at here.
>
Right using "object" might be misleading. On the other hand,
--shared/--static are well known and dead-obvious.
Plus it gives us flexibility to sweep more things under it, if we get
some funky stuff in the future.

Can I buy you on the different name?
Emil
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH 3/5] scanner: introduce --object-type option

[Jonas Ådahl]

On Wed, Jan 24, 2018 at 07:15:09PM +, Emil Velikov wrote:
> On 24 January 2018 at 18:20, Derek Foreman  wrote:
> > On 2018-01-22 09:30 AM, Emil Velikov wrote:
> >>
> >> On 22 August 2017 at 14:02, Emil Velikov  wrote:
> >>>
> >>> On 18 August 2017 at 13:05, Pekka Paalanen  wrote:
> >>>
> >>
> >> The exported configuration would then be:
> >> LOCAL_INTERFACE_DECL=extern
> >> EXTERN_INTERFACE_DECL=extern
> >> LOCAL_INTERFACE_DEF=WL_EXPORT
> >>
> >> That would be far too flexible and no-one would use it right, right?
> >>
> > I did not introduce separate tokens, since those are (and should be)
> > used _only_ in the .c file.
> > Personally then do not seem necessary, If you prefer we can add them
> > though.
> 
> 
>  Ah, no, that was just a wild idea of something completely different. I
>  meant that the user project would be setting those macros before using
>  scanner-generated files, and if unset, the scanner-emitted code would
>  default to the legacy behaviour. That way there would be no visibility
>  modes in scanner itself. If it's not obviously better, then nevermind.
>  It certainly has a lot more room to go wrong than your proposal.
> 
> 
> >>> I see.
> >>>
> >>> Personally I'd lean towards with my approach for now since it is
> >>> simpler, despite that it provides less flexibility.
> >>> As you pointed out the proposal is a bit more fragile, so might be
> >>> better to avoid until there's a real need for it.
> >>>
> >>>
>  ...
> 
> >> The patch looks pretty much correct to me, if we choose to go this
> >> way.
> >>
> > Glad to hear.
> >
> > I'll let me know once you guys are settled in on the approach, and
> > I'll respin the series with all the comments addressed.
> 
> 
>  Cool, let's see if we can get the name conflict issue solved, and then
>  I'll try to remember to ping you.
> 
> >>> Ack, I'll keep an eye open, just in case.
> >>>
> >> Considering the status of the the name conflict series, should I
> >> re-spin this lot?
> >> I'm more than happy to tweak things - say rename the toggle, etc.
> >
> >
> > I see there were two series proposed to control symbol visibility, yours and
> > Jonas'?
> >
> > Assuming that once we drop the symbol collision issue they both solve the
> > same problems, it would be good if we could focus on one going forward.
> >
> > Is this the chosen one?
> >
> Right, the cover letter [1] covers some of the high-lights/differences.
> As a TL;DR: using static/shared is more common and gives us more
> flexibility for the future.
> 
> So far Pekka is the only person who commented on the series/approach
> and seemed happy.
> I was expecting others to weight in - say Jonas ;-) I'll respin the
> series tomorrow.
> 
> In hindsight --object-type={shared,static} is too much of a mouthful,
> I'll opt for --{static,shared} for v2.

I have no opinion of what variant to land (I'm slightly too lazy to
search for my own, and this is high up the inbox thanks to the replies,
so lets focus on this one).

My only nit is using the term "object-type". I think refering to it as
"visibility" ("symbol visibility" if wanting to be extra verbose) where
one can say 'export', 'static' or 'private' is more accurate.

"objects" are a Wayland protocol thing and that is not what we are
poking at here.


Jonas

> 
> -Emil
> 
> [1] https://patchwork.freedesktop.org/series/27939/
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH 3/5] scanner: introduce --object-type option

[Emil Velikov]

On 24 January 2018 at 18:20, Derek Foreman  wrote:
> On 2018-01-22 09:30 AM, Emil Velikov wrote:
>>
>> On 22 August 2017 at 14:02, Emil Velikov  wrote:
>>>
>>> On 18 August 2017 at 13:05, Pekka Paalanen  wrote:
>>>
>>
>> The exported configuration would then be:
>> LOCAL_INTERFACE_DECL=extern
>> EXTERN_INTERFACE_DECL=extern
>> LOCAL_INTERFACE_DEF=WL_EXPORT
>>
>> That would be far too flexible and no-one would use it right, right?
>>
> I did not introduce separate tokens, since those are (and should be)
> used _only_ in the .c file.
> Personally then do not seem necessary, If you prefer we can add them
> though.


 Ah, no, that was just a wild idea of something completely different. I
 meant that the user project would be setting those macros before using
 scanner-generated files, and if unset, the scanner-emitted code would
 default to the legacy behaviour. That way there would be no visibility
 modes in scanner itself. If it's not obviously better, then nevermind.
 It certainly has a lot more room to go wrong than your proposal.


>>> I see.
>>>
>>> Personally I'd lean towards with my approach for now since it is
>>> simpler, despite that it provides less flexibility.
>>> As you pointed out the proposal is a bit more fragile, so might be
>>> better to avoid until there's a real need for it.
>>>
>>>
 ...

>> The patch looks pretty much correct to me, if we choose to go this
>> way.
>>
> Glad to hear.
>
> I'll let me know once you guys are settled in on the approach, and
> I'll respin the series with all the comments addressed.


 Cool, let's see if we can get the name conflict issue solved, and then
 I'll try to remember to ping you.

>>> Ack, I'll keep an eye open, just in case.
>>>
>> Considering the status of the the name conflict series, should I
>> re-spin this lot?
>> I'm more than happy to tweak things - say rename the toggle, etc.
>
>
> I see there were two series proposed to control symbol visibility, yours and
> Jonas'?
>
> Assuming that once we drop the symbol collision issue they both solve the
> same problems, it would be good if we could focus on one going forward.
>
> Is this the chosen one?
>
Right, the cover letter [1] covers some of the high-lights/differences.
As a TL;DR: using static/shared is more common and gives us more
flexibility for the future.

So far Pekka is the only person who commented on the series/approach
and seemed happy.
I was expecting others to weight in - say Jonas ;-) I'll respin the
series tomorrow.

In hindsight --object-type={shared,static} is too much of a mouthful,
I'll opt for --{static,shared} for v2.

-Emil

[1] https://patchwork.freedesktop.org/series/27939/
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH 3/5] scanner: introduce --object-type option

[Derek Foreman]

On 2018-01-22 09:30 AM, Emil Velikov wrote:

On 22 August 2017 at 14:02, Emil Velikov  wrote:

On 18 August 2017 at 13:05, Pekka Paalanen  wrote:



The exported configuration would then be:
LOCAL_INTERFACE_DECL=extern
EXTERN_INTERFACE_DECL=extern
LOCAL_INTERFACE_DEF=WL_EXPORT

That would be far too flexible and no-one would use it right, right?


I did not introduce separate tokens, since those are (and should be)
used _only_ in the .c file.
Personally then do not seem necessary, If you prefer we can add them though.


Ah, no, that was just a wild idea of something completely different. I
meant that the user project would be setting those macros before using
scanner-generated files, and if unset, the scanner-emitted code would
default to the legacy behaviour. That way there would be no visibility
modes in scanner itself. If it's not obviously better, then nevermind.
It certainly has a lot more room to go wrong than your proposal.



I see.

Personally I'd lean towards with my approach for now since it is
simpler, despite that it provides less flexibility.
As you pointed out the proposal is a bit more fragile, so might be
better to avoid until there's a real need for it.



...


The patch looks pretty much correct to me, if we choose to go this way.


Glad to hear.

I'll let me know once you guys are settled in on the approach, and
I'll respin the series with all the comments addressed.


Cool, let's see if we can get the name conflict issue solved, and then
I'll try to remember to ping you.


Ack, I'll keep an eye open, just in case.


Considering the status of the the name conflict series, should I
re-spin this lot?
I'm more than happy to tweak things - say rename the toggle, etc.


I see there were two series proposed to control symbol visibility, yours 
and Jonas'?


Assuming that once we drop the symbol collision issue they both solve 
the same problems, it would be good if we could focus on one going forward.


Is this the chosen one?

Thanks,
Derek


Thanks
Emil
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel



___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH 3/5] scanner: introduce --object-type option

[Emil Velikov]

On 22 August 2017 at 14:02, Emil Velikov  wrote:
> On 18 August 2017 at 13:05, Pekka Paalanen  wrote:
>
>>> >
>>> > The exported configuration would then be:
>>> > LOCAL_INTERFACE_DECL=extern
>>> > EXTERN_INTERFACE_DECL=extern
>>> > LOCAL_INTERFACE_DEF=WL_EXPORT
>>> >
>>> > That would be far too flexible and no-one would use it right, right?
>>> >
>>> I did not introduce separate tokens, since those are (and should be)
>>> used _only_ in the .c file.
>>> Personally then do not seem necessary, If you prefer we can add them though.
>>
>> Ah, no, that was just a wild idea of something completely different. I
>> meant that the user project would be setting those macros before using
>> scanner-generated files, and if unset, the scanner-emitted code would
>> default to the legacy behaviour. That way there would be no visibility
>> modes in scanner itself. If it's not obviously better, then nevermind.
>> It certainly has a lot more room to go wrong than your proposal.
>>
>>
> I see.
>
> Personally I'd lean towards with my approach for now since it is
> simpler, despite that it provides less flexibility.
> As you pointed out the proposal is a bit more fragile, so might be
> better to avoid until there's a real need for it.
>
>
>> ...
>>
>>> > The patch looks pretty much correct to me, if we choose to go this way.
>>> >
>>> Glad to hear.
>>>
>>> I'll let me know once you guys are settled in on the approach, and
>>> I'll respin the series with all the comments addressed.
>>
>> Cool, let's see if we can get the name conflict issue solved, and then
>> I'll try to remember to ping you.
>>
> Ack, I'll keep an eye open, just in case.
>
Considering the status of the the name conflict series, should I
re-spin this lot?
I'm more than happy to tweak things - say rename the toggle, etc.

Thanks
Emil
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH 3/5] scanner: introduce --object-type option

[Emil Velikov]

On 18 August 2017 at 13:05, Pekka Paalanen  wrote:

>> >
>> > The exported configuration would then be:
>> > LOCAL_INTERFACE_DECL=extern
>> > EXTERN_INTERFACE_DECL=extern
>> > LOCAL_INTERFACE_DEF=WL_EXPORT
>> >
>> > That would be far too flexible and no-one would use it right, right?
>> >
>> I did not introduce separate tokens, since those are (and should be)
>> used _only_ in the .c file.
>> Personally then do not seem necessary, If you prefer we can add them though.
>
> Ah, no, that was just a wild idea of something completely different. I
> meant that the user project would be setting those macros before using
> scanner-generated files, and if unset, the scanner-emitted code would
> default to the legacy behaviour. That way there would be no visibility
> modes in scanner itself. If it's not obviously better, then nevermind.
> It certainly has a lot more room to go wrong than your proposal.
>
>
I see.

Personally I'd lean towards with my approach for now since it is
simpler, despite that it provides less flexibility.
As you pointed out the proposal is a bit more fragile, so might be
better to avoid until there's a real need for it.


> ...
>
>> > The patch looks pretty much correct to me, if we choose to go this way.
>> >
>> Glad to hear.
>>
>> I'll let me know once you guys are settled in on the approach, and
>> I'll respin the series with all the comments addressed.
>
> Cool, let's see if we can get the name conflict issue solved, and then
> I'll try to remember to ping you.
>
Ack, I'll keep an eye open, just in case.

Thanks
Emil
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH 3/5] scanner: introduce --object-type option

[Pekka Paalanen]

On Fri, 28 Jul 2017 13:13:48 +0100
Emil Velikov  wrote:

> On 27 July 2017 at 14:25, Pekka Paalanen  wrote:
> > On Wed, 26 Jul 2017 14:56:19 +0100
> > Emil Velikov  wrote:
> >  
> >> From: Emil Velikov 
> >>
> >> The option is used to indicate how the code will be used - would it be a
> >> part of shared or static one.
> >>
> >> In the former case one needs to export the specific symbols, although
> >> normally people want to statically build the protocol code into their
> >> project.
> >>
> >> If the option is missing a warning is emitted, to point people and do
> >> the right thing.
> >>
> >> Signed-off-by: Emil Velikov 
> >> ---
> >>  src/scanner.c | 61 
> >> ++-
> >>  1 file changed, 56 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/src/scanner.c b/src/scanner.c
> >> index c345ed6..cc45b74 100644
> >> --- a/src/scanner.c
> >> +++ b/src/scanner.c
> >> @@ -57,6 +57,11 @@ enum side {
> >>   SERVER,
> >>  };
> >>
> >> +enum object_type {
> >> + SHARED,
> >> + STATIC,
> >> +};  
> >
> > Hi,
> >
> > I could go with this as well, as long as we have some solution to the
> > original problem of conflicting interface names that Jonas is trying to
> > solve.
> >  
> i think the original patch from Jonas, addressed that issue nicely.
> Let's keep that in the respective thread.

Hi Emil,

very good, let's continue that there.


...

> >>  static void
> >> -emit_code(struct protocol *protocol)
> >> +emit_code(struct protocol *protocol, enum object_type obj_type)
> >>  {
> >> + const char *symbol_visibility;
> >>   struct interface *i, *next;
> >>   struct wl_array types;
> >>   char **p, *prev;
> >> @@ -1728,6 +1740,19 @@ emit_code(struct protocol *protocol)
> >>  "#include \n"
> >>  "#include \"wayland-util.h\"\n\n");
> >>
> >> + /* When building a shared library symbols must be exported, otherwise
> >> +  * we want to have the symbols hidden. */
> >> + if (obj_type == STATIC) {
> >> + symbol_visibility = "WL_PRIVATE";
> >> + printf("#if defined(__GNUC__) && __GNUC__ >= 4\n"
> >> +"#define WL_PRIVATE __attribute__ 
> >> ((visibility(\"hidden\")))\n"
> >> +"#else\n"
> >> +"#define WL_PRIVATE\n"
> >> +"#endif\n\n");
> >> + } else {
> >> + symbol_visibility = "WL_EXPORT";
> >> + }  
> >
> > I wonder... would it be any better to just replace 'WL_EXPORT' and
> > 'extern' with tokens:
> >
> > - LOCAL_INTERFACE_DECL for declarations of symbols that will also be
> >   defined in the generated code file
> >
> > - EXTERN_INTERFACE_DECL for declarations of symbols that the generated
> >   files will need but the code file will not define, that is,
> >   interfaces defined on other XML files.
> >
> > - LOCAL_INTERFACE_DEF for symbol definitions in the generated code file.
> >
> > The exported configuration would then be:
> > LOCAL_INTERFACE_DECL=extern
> > EXTERN_INTERFACE_DECL=extern
> > LOCAL_INTERFACE_DEF=WL_EXPORT
> >
> > That would be far too flexible and no-one would use it right, right?
> >  
> I did not introduce separate tokens, since those are (and should be)
> used _only_ in the .c file.
> Personally then do not seem necessary, If you prefer we can add them though.

Ah, no, that was just a wild idea of something completely different. I
meant that the user project would be setting those macros before using
scanner-generated files, and if unset, the scanner-emitted code would
default to the legacy behaviour. That way there would be no visibility
modes in scanner itself. If it's not obviously better, then nevermind.
It certainly has a lot more room to go wrong than your proposal.


...

> > The patch looks pretty much correct to me, if we choose to go this way.
> >  
> Glad to hear.
> 
> I'll let me know once you guys are settled in on the approach, and
> I'll respin the series with all the comments addressed.

Cool, let's see if we can get the name conflict issue solved, and then
I'll try to remember to ping you.


Thanks,
pq


pgpSXRXjhA2ik.pgp
Description: OpenPGP digital signature
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH 3/5] scanner: introduce --object-type option

[Emil Velikov]

On 27 July 2017 at 14:25, Pekka Paalanen  wrote:
> On Wed, 26 Jul 2017 14:56:19 +0100
> Emil Velikov  wrote:
>
>> From: Emil Velikov 
>>
>> The option is used to indicate how the code will be used - would it be a
>> part of shared or static one.
>>
>> In the former case one needs to export the specific symbols, although
>> normally people want to statically build the protocol code into their
>> project.
>>
>> If the option is missing a warning is emitted, to point people and do
>> the right thing.
>>
>> Signed-off-by: Emil Velikov 
>> ---
>>  src/scanner.c | 61 
>> ++-
>>  1 file changed, 56 insertions(+), 5 deletions(-)
>>
>> diff --git a/src/scanner.c b/src/scanner.c
>> index c345ed6..cc45b74 100644
>> --- a/src/scanner.c
>> +++ b/src/scanner.c
>> @@ -57,6 +57,11 @@ enum side {
>>   SERVER,
>>  };
>>
>> +enum object_type {
>> + SHARED,
>> + STATIC,
>> +};
>
> Hi,
>
> I could go with this as well, as long as we have some solution to the
> original problem of conflicting interface names that Jonas is trying to
> solve.
>
i think the original patch from Jonas, addressed that issue nicely.
Let's keep that in the respective thread.

>> +
>>  static int
>>  usage(int ret)
>>  {
>> @@ -70,6 +75,11 @@ usage(int ret)
>>   fprintf(stderr, "-h,  --help  display this help 
>> and exit.\n"
>>   "-v,  --version   print the wayland 
>> library version that\n"
>>   " the scanner was 
>> built against.\n"
>> + "-t,  --object-type=[static,shared]\n"
>> + " How is the resulting 
>> code going to be built/used\n"
>> + " static - standalone 
>> static object, used internally\n"
>> + " shared - shared 
>> library, to be used by multiple projects\n"
>> + " Using static is 
>> highly recommened.\n"
>>   "-c,  --include-core-only include the core 
>> version of the headers,\n"
>>   " that is e.g. 
>> wayland-client-core.h instead\n"
>>   " of 
>> wayland-client.h.\n");
>> @@ -1712,9 +1722,11 @@ emit_messages(struct wl_list *message_list,
>>   printf("};\n\n");
>>  }
>>
>> +
>>  static void
>> -emit_code(struct protocol *protocol)
>> +emit_code(struct protocol *protocol, enum object_type obj_type)
>>  {
>> + const char *symbol_visibility;
>>   struct interface *i, *next;
>>   struct wl_array types;
>>   char **p, *prev;
>> @@ -1728,6 +1740,19 @@ emit_code(struct protocol *protocol)
>>  "#include \n"
>>  "#include \"wayland-util.h\"\n\n");
>>
>> + /* When building a shared library symbols must be exported, otherwise
>> +  * we want to have the symbols hidden. */
>> + if (obj_type == STATIC) {
>> + symbol_visibility = "WL_PRIVATE";
>> + printf("#if defined(__GNUC__) && __GNUC__ >= 4\n"
>> +"#define WL_PRIVATE __attribute__ 
>> ((visibility(\"hidden\")))\n"
>> +"#else\n"
>> +"#define WL_PRIVATE\n"
>> +"#endif\n\n");
>> + } else {
>> + symbol_visibility = "WL_EXPORT";
>> + }
>
> I wonder... would it be any better to just replace 'WL_EXPORT' and
> 'extern' with tokens:
>
> - LOCAL_INTERFACE_DECL for declarations of symbols that will also be
>   defined in the generated code file
>
> - EXTERN_INTERFACE_DECL for declarations of symbols that the generated
>   files will need but the code file will not define, that is,
>   interfaces defined on other XML files.
>
> - LOCAL_INTERFACE_DEF for symbol definitions in the generated code file.
>
> The exported configuration would then be:
> LOCAL_INTERFACE_DECL=extern
> EXTERN_INTERFACE_DECL=extern
> LOCAL_INTERFACE_DEF=WL_EXPORT
>
> That would be far too flexible and no-one would use it right, right?
>
I did not introduce separate tokens, since those are (and should be)
used _only_ in the .c file.
Personally then do not seem necessary, If you prefer we can add them though.

>> +
>>   wl_array_init();
>>   wl_list_for_each(i, >interface_list, link) {
>>   emit_types_forward_declarations(protocol, >request_list, 
>> );
>> @@ -1757,10 +1782,10 @@ emit_code(struct protocol *protocol)
>>   emit_messages(>request_list, i, "requests");
>>   emit_messages(>event_list, i, "events");
>>
>> - printf("WL_EXPORT const struct wl_interface "
>> + printf("%s const struct wl_interface "
>>  "%s_interface = {\n"

Re: [PATCH 3/5] scanner: introduce --object-type option

[Pekka Paalanen]

On Wed, 26 Jul 2017 14:56:19 +0100
Emil Velikov  wrote:

> From: Emil Velikov 
> 
> The option is used to indicate how the code will be used - would it be a
> part of shared or static one.
> 
> In the former case one needs to export the specific symbols, although
> normally people want to statically build the protocol code into their
> project.
> 
> If the option is missing a warning is emitted, to point people and do
> the right thing.
> 
> Signed-off-by: Emil Velikov 
> ---
>  src/scanner.c | 61 
> ++-
>  1 file changed, 56 insertions(+), 5 deletions(-)
> 
> diff --git a/src/scanner.c b/src/scanner.c
> index c345ed6..cc45b74 100644
> --- a/src/scanner.c
> +++ b/src/scanner.c
> @@ -57,6 +57,11 @@ enum side {
>   SERVER,
>  };
>  
> +enum object_type {
> + SHARED,
> + STATIC,
> +};

Hi,

I could go with this as well, as long as we have some solution to the
original problem of conflicting interface names that Jonas is trying to
solve.

> +
>  static int
>  usage(int ret)
>  {
> @@ -70,6 +75,11 @@ usage(int ret)
>   fprintf(stderr, "-h,  --help  display this help and 
> exit.\n"
>   "-v,  --version   print the wayland 
> library version that\n"
>   " the scanner was built 
> against.\n"
> + "-t,  --object-type=[static,shared]\n"
> + " How is the resulting 
> code going to be built/used\n"
> + " static - standalone 
> static object, used internally\n"
> + " shared - shared 
> library, to be used by multiple projects\n"
> + " Using static is 
> highly recommened.\n"
>   "-c,  --include-core-only include the core 
> version of the headers,\n"
>   " that is e.g. 
> wayland-client-core.h instead\n"
>   " of 
> wayland-client.h.\n");
> @@ -1712,9 +1722,11 @@ emit_messages(struct wl_list *message_list,
>   printf("};\n\n");
>  }
>  
> +
>  static void
> -emit_code(struct protocol *protocol)
> +emit_code(struct protocol *protocol, enum object_type obj_type)
>  {
> + const char *symbol_visibility;
>   struct interface *i, *next;
>   struct wl_array types;
>   char **p, *prev;
> @@ -1728,6 +1740,19 @@ emit_code(struct protocol *protocol)
>  "#include \n"
>  "#include \"wayland-util.h\"\n\n");
>  
> + /* When building a shared library symbols must be exported, otherwise
> +  * we want to have the symbols hidden. */
> + if (obj_type == STATIC) {
> + symbol_visibility = "WL_PRIVATE";
> + printf("#if defined(__GNUC__) && __GNUC__ >= 4\n"
> +"#define WL_PRIVATE __attribute__ 
> ((visibility(\"hidden\")))\n"
> +"#else\n"
> +"#define WL_PRIVATE\n"
> +"#endif\n\n");
> + } else {
> + symbol_visibility = "WL_EXPORT";
> + }

I wonder... would it be any better to just replace 'WL_EXPORT' and
'extern' with tokens:

- LOCAL_INTERFACE_DECL for declarations of symbols that will also be
  defined in the generated code file

- EXTERN_INTERFACE_DECL for declarations of symbols that the generated
  files will need but the code file will not define, that is,
  interfaces defined on other XML files.

- LOCAL_INTERFACE_DEF for symbol definitions in the generated code file.

The exported configuration would then be:
LOCAL_INTERFACE_DECL=extern
EXTERN_INTERFACE_DECL=extern
LOCAL_INTERFACE_DEF=WL_EXPORT

That would be far too flexible and no-one would use it right, right?

> +
>   wl_array_init();
>   wl_list_for_each(i, >interface_list, link) {
>   emit_types_forward_declarations(protocol, >request_list, 
> );
> @@ -1757,10 +1782,10 @@ emit_code(struct protocol *protocol)
>   emit_messages(>request_list, i, "requests");
>   emit_messages(>event_list, i, "events");
>  
> - printf("WL_EXPORT const struct wl_interface "
> + printf("%s const struct wl_interface "
>  "%s_interface = {\n"
>  "\t\"%s\", %d,\n",
> -i->name, i->name, i->version);
> +symbol_visibility, i->name, i->name, i->version);
>  
>   if (!wl_list_empty(>request_list))
>   printf("\t%d, %s_requests,\n",
> @@ -1790,6 +1815,24 @@ free_protocol(struct protocol *protocol)
>   free_description(protocol->description);
>  }
>  
> +static enum object_type
> +parse_obj_type(const char *obj_type_str)
> +{
> + if