On Wed, Oct 21, 2015 at 02:23:53PM +0100, Auke Booij wrote: > On 20 October 2015 at 08:57, Bryce Harrington <br...@osg.samsung.com> wrote: > > On Mon, Oct 19, 2015 at 11:21:25PM +0100, Auke Booij wrote: > >> Signed-off-by: Auke Booij <a...@tulcod.com> > >> --- > >> src/scanner.c | 70 > >> +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > >> 1 file changed, 70 insertions(+) > >> > >> diff --git a/src/scanner.c b/src/scanner.c > >> index f456aa5..9856475 100644 > >> --- a/src/scanner.c > >> +++ b/src/scanner.c > >> @@ -128,6 +128,7 @@ struct arg { > >> char *interface_name; > >> struct wl_list link; > >> char *summary; > >> + char *enumeration_name; > >> }; > >> > >> struct enumeration { > >> @@ -136,6 +137,7 @@ struct enumeration { > >> struct wl_list entry_list; > >> struct wl_list link; > >> struct description *description; > >> + int bitfield; > > > > Looks like this code only sets this to 0 or 1; may as well make this bool. > > Other places in scanner.c use bool, so seems to be perfectly allowable. > > Sure, I will change this. > > >> }; > >> > >> struct entry { > >> @@ -540,6 +542,8 @@ start_element(void *data, const char *element_name, > >> const char **atts) > >> const char *summary = NULL; > >> const char *since = NULL; > >> const char *allow_null = NULL; > >> + const char *enumeration_name = NULL; > >> + const char *bitfield = NULL; > >> int i, version = 0; > >> > >> ctx->loc.line_number = XML_GetCurrentLineNumber(ctx->parser); > >> @@ -562,6 +566,10 @@ start_element(void *data, const char *element_name, > >> const char **atts) > >> since = atts[i + 1]; > >> if (strcmp(atts[i], "allow-null") == 0) > >> allow_null = atts[i + 1]; > >> + if (strcmp(atts[i], "enum") == 0) > >> + enumeration_name = atts[i + 1]; > >> + if (strcmp(atts[i], "bitfield") == 0) > >> + bitfield = atts[i + 1]; > >> } > >> > >> ctx->character_data_length = 0; > >> @@ -655,6 +663,14 @@ start_element(void *data, const char *element_name, > >> const char **atts) > >> "allow-null is only valid for objects, > >> strings, and arrays"); > >> } > >> > >> + if (enumeration_name == NULL || strcmp(enumeration_name, "") > >> == 0) > >> + arg->enumeration_name = NULL; > >> + else > >> + arg->enumeration_name = xstrdup(enumeration_name); > >> + > >> + if (allow_null != NULL && !is_nullable_type(arg)) > >> + fail(&ctx->loc, "allow-null is only valid for > >> objects, strings, and arrays"); > >> + > >> if (summary) > >> arg->summary = xstrdup(summary); > >> > >> @@ -665,6 +681,14 @@ start_element(void *data, const char *element_name, > >> const char **atts) > >> fail(&ctx->loc, "no enum name given"); > >> > >> enumeration = create_enumeration(name); > >> + > >> + if (bitfield == NULL || strcmp(bitfield, "false") == 0) > >> + enumeration->bitfield = 0; > >> + else if (strcmp(bitfield, "true") == 0) > >> + enumeration->bitfield =1; > > > > Would it be worth allowing True/False as well? > > Well this might be worth thinking about a bit more. I don't think > there are any boolean fields in other wayland configuration (correct > me if I'm wrong), so whatever we choose, it will set a precedence. > Should I also allow 0/1? Yes/No? (Xorg seems to be rather lax in all > this.)
My rationale here is that some programming languages (e.g. Python) use the capitalized version, so especially as this feature is described as targeting language binders, it might remove a tiny irritation for folks who have it in finger muscle memory. A very small thing but it costs us little. From this rationale, Yes/No may be superfluous, as I don't think there's any programming languages that require those terms for booleans. Seems little harm, but perhaps this is another thing that'd be easier to add than to remove so maybe wait until we spot a use for it in practice? 0/1 is a good idea, but I know there are enough situations in the wild where the meanings are reversed that disallowing it might actually prevent some rare but difficult to diagnose bugs. Similarly, supporting numbers for this type of field might circumvent various languages typechecking abilities; probably not a big deal in this particular case but like you say, we would be wise to consider precidence setting... > >> + else > >> + fail(&ctx->loc, "invalid value for bitfield > >> attribute (%s)", bitfield); > > > > May as well say here that only true/false are accepted in bitfields. > > Might save anyone that hits this from a trip to the documentation. > > I will fix this. Bryce _______________________________________________ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel