On 21 October 2015 at 19:13, Bryce Harrington <[email protected]> wrote: > On Wed, Oct 21, 2015 at 02:23:53PM +0100, Auke Booij wrote: >> On 20 October 2015 at 08:57, Bryce Harrington <[email protected]> wrote: >> > On Mon, Oct 19, 2015 at 11:21:25PM +0100, Auke Booij wrote: >> >> Signed-off-by: Auke Booij <[email protected]> >> >> --- >> >> 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
I was about to fix all three points you brought up, when I realized there already is a precedence: allow-null. Its internal type in scanner.c can be found in struct arg; it is declared as "int nullable;". The corresponding error message is the same as for bitfield. It only allows lower-case "true" and "false". So I did this in an attempt to be consistent. Now I'm not sure what to do, please advise. _______________________________________________ wayland-devel mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/wayland-devel
