Created attachment 121705 [details] [review]
v1

Initial take: compiles but misses a little bit of love in the typelib part. Failing in strange ways when compiling the GLib typelib.

Considering that typelib is platform dependent, and the value type is already known, any reason to not write the native value (or offset to string) directly in the typelib and do parsing only at compile time?


Either way, IMO a better API would be something more like:

gboolean
g_arg_info_default_value(GIArgInfo *info, GArgument *arg)
{
  if (!blob->default_value)
    return FALSE;

  if (type is string)
    arg->v_pointer = g_typelib_get_string (..);
  else if (type is double)
    arg->v_double = ..;
  ...
  return TRUE;
}


I think magic '"NULL" (string) means NULL (pointer)' in band values should be avoided, and something like default-null="1" would be better. But maybe that's just me.

Created attachment 193705 [details] [review]
scanner: Add support for the default annotation

This is the annotation scanner/gir generation part.

There is still one question remaining for the scanner: should we allow default values for a parameter when there is another parameter after it that does not have a default value? Are there languages in which that would make sense? For sure that is forbidden in python.

So my thought on adding this to the typelib is that we:

* Take one of the reserved bits in GIArgInfo
* If set, that means we look into a new section in the typelib (like the directory index section which was added in commit 24a2b4c1543798fe61fd6e3667b12e44766740ae)
* The new section has a mapping from C symbol,param offset to a ConstantBlob

To make this efficient we may need to do something like what's done for the directory index section and make a hash table out of it.

An alternative of it would be:

 * Take one of the reserved bits in GIArgInfo
 * If set, that means we look into an array of "ValueBlobs" appended to SignatureBlob
 * For size efficiency, we have a SignatureBlob appended iff the corresponding ArgInfo has a "has_default_value" bit set, a separate value blob index/counter needs to be maintained to know where to look in the ValueBlob array.

ValueBlob would be something simple like:
typedef struct {
  guint32 size;
  guint32 offset;
} ValueBlob;

I don't think we need more info because we already have the type info in the ArgBlob.

Pros: the lookup would likely be faster as we don't need to calculate any hash
Cons:
 - it might be a bit inconsistent to store argument-related info outside of ArgBlob
 - need to change the size of SignatureBlob and the ValueBlob array won't be in a constant offset in SignatureBlob; I'm not sure whether this is a source of problem or not

Hmm.  It looks to me actually like ConstantBlob as is now is broken for non-string values?  

girnode.c:815
	/* FIXME non-string values */
	size += ALIGN_VALUE (strlen (constant->value) + 1, 4);

The size/offset pairing to me looks weird to me.  In g_constant_info_get_value() we do:

      if (blob->type.flags.pointer)
	value->v_pointer = g_memdup (&rinfo->typelib->data[blob->offset], blob->size);

But we could just do g_strdup() assuming we only support UTF-8 constants.

Oh...it looks like we're storing 32 bit constants in the offset directly, and we're broken for 64 bit values (guint64, double).

See also https://bugzilla.gnome.org/show_bug.cgi?id=654069

So we may need to drain that swamp first.

I think instead of a pair of size/offset, what we want is just a guint64.  If the type is a string/filename, then it's an offset.  Otherwise we read the value directly.

Leaving aside the ConstantBlob brokenness, I think your plan with ValueBlob is fine, but it'd be:

ValueBlob {
  guint16 tag;
  guint64 value
};

or something like that.  I'm pretty sure we can expand SignatureBlob; it's not part of any array.  Having it be a dynamic offset is OK.

Do we really need the tag part? Type info is already available from the ArgBlob (in .arg_type).
I think I will use a simple guint64 for now.

It seems like the hold up here is the typelib bits... would it be possible to get the GIR part pushed by itself?  It would be great to be able to make use of this in Vala.

This would be useful for g_locale_from_utf8().
A binding would convert its representation of a Unicode string for @utf8string (correctly auto-annotated as type utf8), but its clients shouldn't be forced to supply the correct value for @len.

[Mass-moving gobject-introspection tickets to its own Bugzilla product - see bug 708029. Mass-filter your bugmail for this message: introspection20150207 ]

I have need of this myself in AppStreamGlib, although this bug looks pretty old and forgotten about :(

I have need of this for Rust bindings

(In reply to Alberto Ruiz from comment #14)
> I have need of this for Rust bindings

A good start would be:

 - rebase attachment 193705 [details] [review] onto current master
 - split the GIR and typelib bits into two separate commits

Both Vala and Rust would consume the GIR anyway, AFAIR, so they would immediately get the benefit of a default value; it would also help documentation generators like HotDoc that use GIR as a source.

Then we can look at how to implement this in the typelib for dynamic languages.

De-ASSIGN this bug, since it hasn't seen any activity since 2011.

-- GitLab Migration Automatic Message --

This bug has been migrated to GNOME's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/gobject-introspection/issues/4.