On Thu, Oct 22, 2015 at 04:25:45PM +0100, Auke Booij wrote: > 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.
Ah good find. Probably should be changed there as well. But there's little point in further complicating this patch. Leave it as true/false here. If it's worth supporting True/False there's no reason it can't be done as a followup. Bryce _______________________________________________ wayland-devel mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/wayland-devel
