Created attachment 137459 [details] [review]
steal () method for the Map interface

I would prefer that we change the signature of the remove method.

Do not know how we can make a transition to that however...

I have another proposal. Adding a default nullable out parameter to remove:

public bool remove(K key, out V? value = null);

This would not break existing code, and give the possibility to user to retrieve the removed value, or not.

I don't think that's a good idea. The principle is nice, but I hate unnecessary out parameters.

Imagine this difference:

  var obj = map.steal (key);

  Object obj;
  map.remove (key, out obj);

In my opinion, that's a very ugly usage. IMHO, out parameters should be avoided wherever possible.

Also, I think remove() should do just what it's meant to do - remove. Defining a new method for removing-and-retrieving seems much better to me, and you don't have to solve any API breakage.

By the way, this wouldn't work unless you write it in C or fix the compiler. The reason is that in C, out parameter is a pointer to a variable, and Vala doesn't check it's validity before using it.

(In reply to comment #4)
> I don't think that's a good idea. The principle is nice, but I hate
> unnecessary out parameters.
> 
> Imagine this difference:
> 
>   var obj = map.steal (key);
> 
>   Object obj;
>   map.remove (key, out obj);

Keep in mind that the actual remove() method returns a boolean to indicate success of the removal.

> In my opinion, that's a very ugly usage. IMHO, out parameters should be
> avoided wherever possible.

In Java, the remove() method return the removed value or null, if unsuccessful.
In C5, the remove() method returns a boolean, and take an additional out parameter to retrieve the remove value if successful.

In the three cases, I prefer the one of C5.

> Also, I think remove() should do just what it's meant to do - remove. Defining
> a new method for removing-and-retrieving seems much better to me, and you
> don't have to solve any API breakage.

Having it optional and defaulted to null *would not break the API*.
One could still call it without the additional out parameter:

if (map.remove(key)) {
   // Do whatever on removal success
}

but one could also retrieve the value and also use the boolean for a test.

MyValueType? value;
if (map.remove(key, out value)) {
   // Do whatever with the value
}

With the steal method I would have to do such thing, which IMHO is more even ugly:

MyValueType? value;
if ((value = map.steal(key)) != null) {
   // Do whatever with the value
}

> By the way, this wouldn't work unless you write it in C or fix the compiler.
> The reason is that in C, out parameter is a pointer to a variable, and Vala
> doesn't check it's validity before using it.

I don't get your point here. Optional defaulted to null out parameters are correctly implemented in Vala (this has been fixed some weeks ago). A simple check like this inside the method ensure that we can write the value in the out parameter:

if (&value != null) {
	value = (owned) (*node)->value;
}

For an example of such a thing in the Valac code itself, see:

http://git.gnome.org/cgit/vala/tree/vapigen/valagirparser.vala#n416

(In reply to comment #5)
> (In reply to comment #4)
> > In my opinion, that's a very ugly usage. IMHO, out parameters should be
> > avoided wherever possible.
> 
> In Java, the remove() method return the removed value or null, if unsuccessful.
> In C5, the remove() method returns a boolean, and take an additional out
> parameter to retrieve the remove value if successful.
> 
> In the three cases, I prefer the one of C5.
> 

Honestly, I think I don't like either way :D

> > Also, I think remove() should do just what it's meant to do - remove. Defining
> > a new method for removing-and-retrieving seems much better to me, and you
> > don't have to solve any API breakage.
> 
> Having it optional and defaulted to null *would not break the API*.
> One could still call it without the additional out parameter:
> 

I'm speaking generally. I'm not that stupid.

> > By the way, this wouldn't work unless you write it in C or fix the compiler.
> > The reason is that in C, out parameter is a pointer to a variable, and Vala
> > doesn't check it's validity before using it.
> 
> I don't get your point here. Optional defaulted to null out parameters are
> correctly implemented in Vala (this has been fixed some weeks ago). A simple
> check like this inside the method ensure that we can write the value in the out
> parameter:
> 
> if (&value != null) {
>         value = (owned) (*node)->value;
> }
> 

Of course! Sorry, in this case I *was* stupid. Testing the address of the argument didn't occur to me somehow.


Anyway, thinking about it a bit more, perhaps the out parameter is not such a bad idea. It would be ugly in cases when you're sure the key is there, or when it doesn't matter, but I have no idea how frequent would such cases be.

Hi Jiří,

(In reply to comment #6)
> [...]
> Honestly, I think I don't like either way :D
> [...]
> I'm speaking generally. I'm not that stupid.

I'm sorry, I did not want to offense you. If you felt in my language I was taking you for an idiot, please ignore that. This is not the case at all.

My only grief is that I just want to avoid bloating the interface with too much methods. After having reading the C5 documentation, and walked again the Java Collection framework, it appears to me there are still lots of other features Gee is missing. I feel we will add some more methods for those in the future. Keeping the removal feature located in one method was thus a good idea for me.

> [...]
> Anyway, thinking about it a bit more, perhaps the out parameter is not such a
> bad idea. It would be ugly in cases when you're sure the key is there, or when
> it doesn't matter, but I have no idea how frequent would such cases be.

OK. Maybe we should let some time go to think more about it. I will implement a patch for the modified remove(). Thus we will be able to compare the two and try them in real situation.

Created attachment 138898 [details] [review]
remove() enhancement proposal to support retrieving of the removed value

This patch requires bug 589236 to be fixed.

(In reply to comment #7)
> Hi Jiří,
> 
> (In reply to comment #6)
> > [...]
> > Honestly, I think I don't like either way :D
> > [...]
> > I'm speaking generally. I'm not that stupid.
> 
> I'm sorry, I did not want to offense you. If you felt in my language I was
> taking you for an idiot, please ignore that. This is not the case at all.
> 

No offense taken. :)

> My only grief is that I just want to avoid bloating the interface with too much
> methods. [...]

Yeah, I understand your concern.

> > [...]
> OK. Maybe we should let some time go to think more about it. I will implement a
> patch for the modified remove(). Thus we will be able to compare the two and
> try them in real situation.
> 

Good. I'll go with your patch from now and see how it works for me. Let's let this discussion rest until there is some more insight to share. :)

(In reply to comment #3)
> I have another proposal. Adding a default nullable out parameter to remove:
> 
> public bool remove(K key, out V? value = null);
> 
> This would not break existing code, and give the possibility to user to
> retrieve the removed value, or not.

This still breaks API (Vala and C API for implementors and just C API for users) and ABI...

(In reply to comment #10)
> This still breaks API (Vala and C API for implementors and just C API for
> users) and ABI...

Humm. Yeah you are right!

Could we consider doing such thing at a new minor version ? I would be glad if we can do such things for 0.2.

commit e21b74622efb59f5e7bbafb8bbb09ea8af1e8931
Author: Didier 'Ptitjes <ptitjes@free.fr>
Date:   Wed Aug 5 10:26:07 2009 +0200

    Enhanced Map removal to optionally retrieve removed value
    
    Fixes bug 587134.