GNOME Bugzilla – Bug 629704
Patches for large enumeration values
Last modified: 2015-02-07 16:46:32 UTC
It's pretty normal to have a flags value of 0x80000000, and when a language binding converts that to a native integer it should appear as 0x80000000 not as -0x80000000. This requires somewhat extensive changes as everything wants to cast enum values to (int). (After spending a ton of time on this I'm not really sure the pay-off was worth the time, but I think the change is probably worthwhile to include now that it's working. Use judgement when reviewing.)
Created attachment 170282 [details] [review] Compute enumeration storage types more accurately Previously we just were sloppy and didn't bother to accurately compute signed/unsigned for enumeration types. But since we expect bindings to decode a field value or function return value from an integer to an enumeration they have know whether an integer value is 0xffffffff or -1, so we need to do the full computation.
Created attachment 170283 [details] [review] Handle enumerations with the full range of signed and unsigned values The C compiler will pick an enumeration type that accomodates the specified values for the enumeration, so ignoring 64-bit enumerations, we can have enumeration values from MININT32 to MAXUINT32. To handle this properly: - Use gint64 for holding eumeration values when scanning - Add a 'unsigned_value' bit to ValueBlob so we can distinguish the int32 vs. uint32 cases in the typelib - Change the return value of g_value_info_get_value() to gint64.
Review of attachment 170282 [details] [review]: This fix is completely straight-forward, I see no reason to hold it back. You asked on IRC if there was some runtime tests, there are a few in tests/repository which loads the gio typelib, would make sense to extend it to test enums from the regress typelib in there as well, but it's not a strict requirement for committing this patch.
Review of attachment 170283 [details] [review]: Minor comments. ::: girepository/gitypelib-internal.h @@ +633,3 @@ * ValueBlob: * @deprecated: Whether this value is deprecated + * @unsigned_value: if set, value is a 32-bit unsigned integer case to gint32 Malformed sentence; just "if set, value is an unsigned 32 bit integer"? ::: giscanner/gdumpparser.py @@ +261,3 @@ + # The scanned member values are more accurate than the introspected values + # because the introspected values have been compressed to (int), even if + # they were unsigned. It's a bit confusing to say "introspected" in this context; I tend to say "gtype data". Should mention GEnumValue's limitation here. @@ +264,3 @@ + previous_values = {} + previous = self._namespace.get(enum_name) + if isinstance(previous, ast.Enum) or isinstance(previous, ast.Bitfield): You can do isinstance(previous, (ast.Enum, ast.Bitfield)) ::: giscanner/giscannermodule.c @@ +152,3 @@ + return PyLong_FromLongLong ((long long)self->symbol->const_int); + else + return PyInt_FromLong ((long)self->symbol->const_int); Why not always use PyLong_FromLongLong ?
(In reply to comment #4) > Review of attachment 170283 [details] [review]: > > Minor comments. > > ::: girepository/gitypelib-internal.h > @@ +633,3 @@ > * ValueBlob: > * @deprecated: Whether this value is deprecated > + * @unsigned_value: if set, value is a 32-bit unsigned integer case to gint32 > > Malformed sentence; just "if set, value is an unsigned 32 bit integer"? Meant to be "cast to". > ::: giscanner/gdumpparser.py > @@ +261,3 @@ > + # The scanned member values are more accurate than the introspected > values > + # because the introspected values have been compressed to (int), even > if > + # they were unsigned. > > It's a bit confusing to say "introspected" in this context; I tend to say > "gtype data". Should mention GEnumValue's limitation here. Rewritten to: # The scanned member values are more accurate than the values that the # we dumped from GEnumValue.value because GEnumValue.value has the # values as a 32-bit signed integer, even if they were unsigned # in the source code. > @@ +264,3 @@ > + previous_values = {} > + previous = self._namespace.get(enum_name) > + if isinstance(previous, ast.Enum) or isinstance(previous, > ast.Bitfield): > > You can do isinstance(previous, (ast.Enum, ast.Bitfield)) Fixed. > ::: giscanner/giscannermodule.c > @@ +152,3 @@ > + return PyLong_FromLongLong ((long long)self->symbol->const_int); > + else > + return PyInt_FromLong ((long)self->symbol->const_int); > > Why not always use PyLong_FromLongLong ? Premature optimization (PyLong is multi-precision not 64-bit so it's not as ridiculous as it might look.) Removed.
Attachment 170282 [details] pushed as 29ec429 - Compute enumeration storage types more accurately Attachment 170283 [details] pushed as 8916db0 - Handle enumerations with the full range of signed and unsigned values
[Mass-moving gobject-introspection tickets to its own Bugzilla product - see bug 708029. Mass-filter your bugmail for this message: introspection20150207 ]