Re: [PATCH wayland 1/4] wayland-util: do not export the wl_map_* API

[Emil Velikov]

Hi Daniel, all,

On 21 November 2016 at 17:32, Daniel Stone  wrote:
> Hi Emil,
>
> On 30 August 2016 at 18:24, Emil Velikov  wrote:
>> From: Emil Velikov 
>>
>> Use only internally and explicitly marked as such with commit
>> cf04b0a18f2 ("Move private definitions and prototypes to new
>> zwayland-private.h")
>>
>> Signed-off-by: Emil Velikov 
>> ---
>> I could not find any users of the API and I doubt there was ever one. If
>> people feel nervous about this, we can keep it.
>
> For the actual series, detaching the discussion from the
> wayland-scanner bits in 0/4:
>
> I think this one is fine, but I'd prefer to merge it at the same time
> as the wayland-util split, if and when that happens.
>
> Patch 2/4 (to move to the -private file) no longer applies, because we
> let this series bitrot for so long. Sorry.
>
No worries, it's not something that crazy of an issue to begin with.

> Patch 4/4 removes whitespace from the other -uninstalled.pc.in files,
> but you add the same whitespace into the wayland-util file introduced
> in 3/4 and don't fix it up in 4/4.
I've addressed all the white space bits, plus fixed the -client one ;-)

> As for the actual libwayland-util split, I'm very much on the fence as
> to whether it's a good idea. Broadly speaking I do like the idea and
> sympathise with the aims, but am not sure how happy distro packagers
> would be with an extra binary package to track. What really worries me
> though, is transient symbol dependencies: at least with the pkg-config
> modifications as-is in 3/4, with wayland-util dropping back to
> Requires.private, I believe we'd see the compiler/linker complaining
> that a project directly using symbols from wayland-util does not
> directly link to it, only transiently via libwayland-{client,server}.
> They could fix that by requiring wayland-util, but then they'd need
> versioned fallbacks, and we've just made it a fair bit harder for
> people to properly link to it.
>
> Did you test with something that only has
> wayland-client/wayland-server (and/or -uninstalled variants) in the
> pkg-config file, directly using wl_list/wl_array/etc, and see if they
> generated any warnings?
>
I've tried to answer your questions with the 3/4 commit message,
although I might have failed.

Tl;Dr; everything is file, see the specifics below.

Since I was too lazy to pull/rebuild something crazy big as KF5/the
Gnome equiv./other, I've did a pretty trivial example
https://github.com/evelikov/wl_link_test

- Builds one executable and one shared (DSO) library.
- Both containing the same code - wl_{list,array}_init
- Explicitly link the DSO w/o undefined symbols
- link each binary against libwayland-{client,server}.so
- no warnings at build/link time
- libwayland-utils.so ends up with NEEDED

Tried the above with and w/o the following (the default to Arch) LDFLAGS
LDFLAGS="-Wl,-O1,--sort-common,--as-needed,-z,relro"

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

Re: [PATCH wayland 1/4] wayland-util: do not export the wl_map_* API

[Daniel Stone]

Hi Emil,

On 30 August 2016 at 18:24, Emil Velikov  wrote:
> From: Emil Velikov 
>
> Use only internally and explicitly marked as such with commit
> cf04b0a18f2 ("Move private definitions and prototypes to new
> zwayland-private.h")
>
> Signed-off-by: Emil Velikov 
> ---
> I could not find any users of the API and I doubt there was ever one. If
> people feel nervous about this, we can keep it.

For the actual series, detaching the discussion from the
wayland-scanner bits in 0/4:

I think this one is fine, but I'd prefer to merge it at the same time
as the wayland-util split, if and when that happens.

Patch 2/4 (to move to the -private file) no longer applies, because we
let this series bitrot for so long. Sorry.

Patch 4/4 removes whitespace from the other -uninstalled.pc.in files,
but you add the same whitespace into the wayland-util file introduced
in 3/4 and don't fix it up in 4/4.

As for the actual libwayland-util split, I'm very much on the fence as
to whether it's a good idea. Broadly speaking I do like the idea and
sympathise with the aims, but am not sure how happy distro packagers
would be with an extra binary package to track. What really worries me
though, is transient symbol dependencies: at least with the pkg-config
modifications as-is in 3/4, with wayland-util dropping back to
Requires.private, I believe we'd see the compiler/linker complaining
that a project directly using symbols from wayland-util does not
directly link to it, only transiently via libwayland-{client,server}.
They could fix that by requiring wayland-util, but then they'd need
versioned fallbacks, and we've just made it a fair bit harder for
people to properly link to it.

Did you test with something that only has
wayland-client/wayland-server (and/or -uninstalled variants) in the
pkg-config file, directly using wl_list/wl_array/etc, and see if they
generated any warnings?

If we could be sure that was not the case, that would soothe my concerns a bit.

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

[PATCH wayland 1/4] wayland-util: do not export the wl_map_* API

[Emil Velikov]

From: Emil Velikov 

Use only internally and explicitly marked as such with commit
cf04b0a18f2 ("Move private definitions and prototypes to new
zwayland-private.h")

Signed-off-by: Emil Velikov 
---
I could not find any users of the API and I doubt there was ever one. If
people feel nervous about this, we can keep it.
---
 src/wayland-util.c | 18 +-
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/src/wayland-util.c b/src/wayland-util.c
index 7467366..0cdbada 100644
--- a/src/wayland-util.c
+++ b/src/wayland-util.c
@@ -176,21 +176,21 @@ union map_entry {
 #define map_entry_get_data(entry) ((void *)((entry).next & ~(uintptr_t)0x3))
 #define map_entry_get_flags(entry) (((entry).next >> 1) & 0x1)
 
-WL_EXPORT void
+void
 wl_map_init(struct wl_map *map, uint32_t side)
 {
memset(map, 0, sizeof *map);
map->side = side;
 }
 
-WL_EXPORT void
+void
 wl_map_release(struct wl_map *map)
 {
wl_array_release(>client_entries);
wl_array_release(>server_entries);
 }
 
-WL_EXPORT uint32_t
+uint32_t
 wl_map_insert_new(struct wl_map *map, uint32_t flags, void *data)
 {
union map_entry *start, *entry;
@@ -222,7 +222,7 @@ wl_map_insert_new(struct wl_map *map, uint32_t flags, void 
*data)
return (entry - start) + base;
 }
 
-WL_EXPORT int
+int
 wl_map_insert_at(struct wl_map *map, uint32_t flags, uint32_t i, void *data)
 {
union map_entry *start;
@@ -250,7 +250,7 @@ wl_map_insert_at(struct wl_map *map, uint32_t flags, 
uint32_t i, void *data)
return 0;
 }
 
-WL_EXPORT int
+int
 wl_map_reserve_new(struct wl_map *map, uint32_t i)
 {
union map_entry *start;
@@ -289,7 +289,7 @@ wl_map_reserve_new(struct wl_map *map, uint32_t i)
return 0;
 }
 
-WL_EXPORT void
+void
 wl_map_remove(struct wl_map *map, uint32_t i)
 {
union map_entry *start;
@@ -313,7 +313,7 @@ wl_map_remove(struct wl_map *map, uint32_t i)
map->free_list = (i << 1) | 1;
 }
 
-WL_EXPORT void *
+void *
 wl_map_lookup(struct wl_map *map, uint32_t i)
 {
union map_entry *start;
@@ -336,7 +336,7 @@ wl_map_lookup(struct wl_map *map, uint32_t i)
return NULL;
 }
 
-WL_EXPORT uint32_t
+uint32_t
 wl_map_lookup_flags(struct wl_map *map, uint32_t i)
 {
union map_entry *start;
@@ -372,7 +372,7 @@ for_each_helper(struct wl_array *entries, 
wl_iterator_func_t func, void *data)
func(map_entry_get_data(*p), data);
 }
 
-WL_EXPORT void
+void
 wl_map_for_each(struct wl_map *map, wl_iterator_func_t func, void *data)
 {
for_each_helper(>client_entries, func, data);
-- 
2.9.0

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