guile-gi icon indicating copy to clipboard operation
guile-gi copied to clipboard

"tree-store:remove?" should be "tree-store:remove!"

Open daym opened this issue 4 years ago • 6 comments

gtk_tree_store_remove gets an pointer to a GtkTreeIter record, which it modifies. Therefore, the procedure name should mark that the procedure is destructive.

The respective gir part is (in gtk+-3.24.20):

      <method name="remove" c:identifier="gtk_tree_store_remove">
        <doc xml:space="preserve"
             filename="gtktreestore.c"
             line="1194">Removes @iter from @tree_store.  After being removed, @iter is set to the
next valid row at that level, or invalidated if it previously pointed to the
last one.</doc>
        <source-position filename="gtktreestore.h" line="99"/>
        <return-value transfer-ownership="none">
          <doc xml:space="preserve"
               filename="gtktreestore.c"
               line="1203">%TRUE if @iter is still valid, %FALSE if not.</doc>
          <type name="gboolean" c:type="gboolean"/>
        </return-value>
        <parameters>
          <instance-parameter name="tree_store" transfer-ownership="none">
            <doc xml:space="preserve"
                 filename="gtktreestore.c"
                 line="1196">A #GtkTreeStore</doc>
            <type name="TreeStore" c:type="GtkTreeStore*"/>
          </instance-parameter>
          <parameter name="iter" transfer-ownership="none">
            <doc xml:space="preserve"
                 filename="gtktreestore.c"
                 line="1197">A valid #GtkTreeIter</doc>
            <type name="TreeIter" c:type="GtkTreeIter*"/>
          </parameter>
        </parameters>
      </method>

... so I don't see how guile-gi could reliably detect that iter is modified by that function.

Even (g_arg_info_get_direction(ai) == GI_DIRECTION_INOUT) is not true.

Filing Gtk bug...

daym avatar Sep 24 '20 09:09 daym

https://gitlab.gnome.org/GNOME/gtk/-/issues/3189

daym avatar Sep 24 '20 09:09 daym

Out of curiosity, would this issue be resolved if you modified the gir as you proposed and used the typelib generated from that?

LordYuuma avatar Sep 25 '20 12:09 LordYuuma

Yes--but https://gitlab.gnome.org/GNOME/gtk/-/issues/3189 states that this new usage of inout is wrong. I asked how to to notice that a procedure modifies its argument in place then. Let's see...

See also: https://gitlab.gnome.org/GNOME/gobject-introspection/-/issues/192 , where it says "Another use-case is in-place modification of input where argument is passed using only one level of indirection."

I checked the functions mentioned in the latter issue 192 in gobject-introspection 1.62.0 and in pango 1.44.7:

  • g_base64_decode_inplace: return-value: transfer-ownership="none"; parameter text: direction="inout" caller-allocates="0" transfer-ownership="full"
  • g_base_info_iterate_attributes: parameter iterator: direction="inout", caller-allocates="0", transfer-ownership="full"; parameter name: direction="out", caller-allocates="0", transfer-ownership="none"; parameter value: direction="out", caller-allocates="0", transfer-ownership="none"
  • g_callable_info_iterate_return_attributes: parameter iterator: direction="inout", caller-allocates="0", transfer-ownership="full"
  • g_signal_emitv: parameter instance_and_params: transfer-ownership="none"; parameter return_value: direction="inout", caller-allocates="0", transfer-ownership="full" optional="1"
  • pango_matrix_transform_pixel_rectangle: parameter rect: direction="inout".
  • pango_matrix_transform_rectangle: parameter rect: direction="inout".

There's also issue 196 which proposes to have an extra "modified-in-place" marker (and https://gitlab.gnome.org/GNOME/gobject-introspection/-/issues/166 for a different one ((inout caller-allocates))), which was closed in favor of inout for the time being. Well, which is it now?

daym avatar Nov 08 '20 07:11 daym

I can see, that they'd object to overloading inout in such a manner, but in my opinion they are in the wrong for not supplying sufficient information to determine whether or not a particular parameter is modified. We already do our best to estimate which procedures are predicative and destructive based on the limited information we have available.

LordYuuma avatar Nov 08 '20 09:11 LordYuuma

https://gitlab.gnome.org/GNOME/gobject-introspection/-/issues/192 proposes to have a new ref annotation in gobject-introspection.

Note that GtkTextView uses const GtkTextIter* for parameters where the iter is not changed (const prefix is also included in c:type in the GIR). So there, one would know that the iter will not be changed by a method.

But GtkTreeModel doesn't use const GtkTreeIter :P

I don't know how it is for the other boxed types--although one could probably use (gi documentation) somehow in order to find out all the affected methods (methods which have any parameter which is a pointer to a record type (maybe only those that are not gobjects--not sure)) and check whether they are detected right.

daym avatar Nov 09 '20 11:11 daym

ref does have the advantage of kinda being tested in Vala already, but its semantics strongly overlap with inout, so I'm not quite certain, whether this is a sufficient solution.

LordYuuma avatar Nov 11 '20 18:11 LordYuuma