On Wed, 26 Jul 2017 16:16:33 +0800 Jonas Ådahl <[email protected]> wrote:
> Add a --visibility flag that enables the user to tweak the visibility > of the symbols generated by wayland-scanner. Three alternatives are > exposed: > > 'export': as has always been done up until now, export the symbols > using WL_EXPORT, making making them exposed externally. This is the > default in order to not break backward compatibility. > > 'compiler-default': use whatever visibility the compiler defaults to. > This is most likely the most visibility that protocol implementations > or users actually wants, as it doesn't expose any unwanted > implementation details. > > 'static': each symbol will only be visible to the compilation unit it > is included in. This means that a protocol implementations and users > needs to include both the 'code' file and either the 'client-header' or > 'server-header' (or both) in the source file implementing or using the > protocol in question. > > Using 'static' is a method to avoid situations where otherwise exposed > symbols of different protocols would conflict, for example if they have > the same interface name. > > When no visibility is specified, 'export' is assumed, but a warning is > printed to stderr, as it is unlikely that 'export' is what is actually > desired. > > Signed-off-by: Jonas Ådahl <[email protected]> > --- > > Changes since v1: > > - Use 'extern' tag also for compiler default declarations. > > - "see --help" message to the warning > > - Added 'what visibility to choose' kind of documentation to --help > > > Makefile.am | 10 ++--- > src/scanner.c | 135 > +++++++++++++++++++++++++++++++++++++++++++++++++++------- > 2 files changed, 125 insertions(+), 20 deletions(-) Hi, patches 2-4 are: Reviewed-by: Pekka Paalanen <[email protected]> Patch 1 looks good, except for one detail we have missed completely and ruins almost everything. > diff --git a/src/scanner.c b/src/scanner.c > index 517068c..3e43f90 100644 > --- a/src/scanner.c > +++ b/src/scanner.c > @@ -1507,11 +1537,14 @@ emit_mainpage_blurb(const struct protocol *protocol, > enum side side) > } > > static void > -emit_header(struct protocol *protocol, enum side side) > +emit_header(struct parse_context *ctx, > + struct protocol *protocol, > + enum side side) > { > struct interface *i, *i_next; > struct wl_array types; > const char *s = (side == SERVER) ? "SERVER" : "CLIENT"; > + const char *symbol_decl_tag = NULL; > char **p, *prev; > > printf("/* Generated by %s %s */\n\n", PROGRAM_NAME, WAYLAND_VERSION); > @@ -1556,6 +1589,16 @@ emit_header(struct protocol *protocol, enum side side) > wl_array_release(&types); > printf("\n"); > > + switch (ctx->visibility) { > + case VISIBILITY_EXPORT: > + case VISIBILITY_COMPILER_DEFAULT: > + symbol_decl_tag = "extern "; > + break; > + case VISIBILITY_STATIC: > + symbol_decl_tag = "static "; > + break; > + } > + > wl_list_for_each(i, &protocol->interface_list, link) { > printf("/**\n" > " * @page page_iface_%s %s\n", > @@ -1575,8 +1618,9 @@ emit_header(struct protocol *protocol, enum side side) > if (i->description && i->description->text) > format_text_to_comment(i->description->text, false); > printf(" */\n"); > - printf("extern const struct wl_interface " > - "%s_interface;\n", i->name); > + printf("%sconst struct wl_interface " > + "%s_interface;\n", > + symbol_decl_tag, i->name); > } > > printf("\n"); > @@ -1713,11 +1757,13 @@ emit_messages(struct wl_list *message_list, > } > > static void > -emit_code(struct protocol *protocol) > +emit_code(struct parse_context *ctx, struct protocol *protocol) > { > struct interface *i, *next; > struct wl_array types; > char **p, *prev; > + const char *visibility_tag = NULL; > + const char *symbol_decl_tag = NULL; > > printf("/* Generated by %s %s */\n\n", PROGRAM_NAME, WAYLAND_VERSION); > > @@ -1728,6 +1774,16 @@ emit_code(struct protocol *protocol) > "#include <stdint.h>\n" > "#include \"wayland-util.h\"\n\n"); > > + switch (ctx->visibility) { > + case VISIBILITY_EXPORT: > + case VISIBILITY_COMPILER_DEFAULT: > + symbol_decl_tag = "extern "; > + break; > + case VISIBILITY_STATIC: > + symbol_decl_tag = "static "; > + break; > + } > + > wl_array_init(&types); > wl_list_for_each(i, &protocol->interface_list, link) { > emit_types_forward_declarations(protocol, &i->request_list, > &types); > @@ -1738,7 +1794,8 @@ emit_code(struct protocol *protocol) > wl_array_for_each(p, &types) { > if (prev && strcmp(*p, prev) == 0) > continue; > - printf("extern const struct wl_interface %s_interface;\n", *p); > + printf("%sconst struct wl_interface %s_interface;\n", > + symbol_decl_tag, *p); In the static case, we emit a thing like: static const struct wl_interface intf_not_here_interface; But that is wrong, because 'intf_not_here' could as well be 'wl_surface', i.e. an interface defined in another XML file. > prev = *p; > } > wl_array_release(&types); > @@ -1752,14 +1809,26 @@ emit_code(struct protocol *protocol) > } > printf("};\n\n"); > > + switch (ctx->visibility) { > + case VISIBILITY_EXPORT: > + visibility_tag = "WL_EXPORT "; > + break; > + case VISIBILITY_COMPILER_DEFAULT: > + visibility_tag = ""; > + break; > + case VISIBILITY_STATIC: > + visibility_tag = "static "; > + break; > + } > wl_list_for_each_safe(i, next, &protocol->interface_list, link) { > > emit_messages(&i->request_list, i, "requests"); > emit_messages(&i->event_list, i, "events"); > > - printf("WL_EXPORT const struct wl_interface " > + printf("%sconst struct wl_interface " > "%s_interface = {\n" > "\t\"%s\", %d,\n", > + visibility_tag, > i->name, i->name, i->version); > > if (!wl_list_empty(&i->request_list)) However, it is curious. $ git ngrep -p intf_not_here -- tests/data/static-small-* tests/data/static-small-client-core.h=46=struct intf_A; tests/data/static-small-client-core.h:47:struct intf_not_here; tests/data/static-small-client-core.h=155=intf_A_rq1(struct intf_A *intf_A, const struct wl_interface *interface, uint32_t version) tests/data/static-small-client-core.h:168:static inline struct intf_not_here * tests/data/static-small-client-core.h=169=intf_A_rq2(struct intf_A *intf_A, const char *str, int32_t i, uint32_t u, wl_fixed_t f, int32_t fd, struct another_intf *obj) tests/data/static-small-client-core.h:174: INTF_A_RQ2, &intf_not_here_interface, NULL, str, i, u, f, fd, obj); tests/data/static-small-client-core.h:176: return (struct intf_not_here *) typed_new; tests/data/static-small-code-core.c=32=static const struct wl_interface another_intf_interface; tests/data/static-small-code-core.c:33:static const struct wl_interface intf_not_here_interface; tests/data/static-small-code-core.c=35=static const struct wl_interface *types[] = { tests/data/static-small-code-core.c:37: &intf_not_here_interface, tests/data/static-small-server-core.h=49=struct intf_A; tests/data/static-small-server-core.h:50:struct intf_not_here; The problematic static const struct wl_interface intf_not_here_interface; is there. But I would have expected to see another one in the client header, because it uses intf_not_here_interface. Instead it seems to rely on including the depended-on protocol's client header first. What to do? Make only local interfaces static and keep external interfaces always as 'extern'? It would break down if one wanted to have two interacting protocol extensions both generated as static, but I suppose that's so far into corner-case territory that we can ignore it? Could we drop the declaration for external interfaces completely, and rely on the user #including the depended-on header, like the client-headers already do? That would be cleaner, except if we do it only in the static case, but OTOH if we did it in all cases, it might break someone's build. Well, given it's a .c file we generate, it would definitely break builds, so we can drop the declarations only in the static case where the code file must be #included anyway. Opinions? Given all this, I'm leaning on giving up hope on landing this for the ongoing release. We need more soaking time for whatever we do. Not quite sure if asking for a second RC would be enough either, and personally I will be on holidays for two weeks starting on Friday. Maybe omitting the declarations for external interfaces in the static case is the way to go? Thanks, pq
pgpRzqSK2klUS.pgp
Description: OpenPGP digital signature
_______________________________________________ wayland-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/wayland-devel
