Project

General

Profile

Actions

Feature #6219

closed

Return value of Hash#store

Added by MartinBosslet (Martin Bosslet) almost 12 years ago. Updated over 3 years ago.

Status:
Feedback
Target version:
-
[ruby-core:43814]

Description

Hash#store returns the value that was just assigned, for example:

h[:a] = b # => b

Does anyone rely on this behavior, are there cases when this becomes handy?

If however the return value is discarded most of the time, I was thinking it
might be beneficial if we would return the previous value of a given key (nil if
none was assigned yet) instead. That way we could assign and check for a collision
in one pass, something that right now can only be done in two separate steps.


Files

0001-Hash-store-return-old-value.patch (6.8 KB) 0001-Hash-store-return-old-value.patch nobu (Nobuyoshi Nakada), 03/30/2012 01:09 AM

Updated by aprescott (Adam Prescott) almost 12 years ago

On Wed, Mar 28, 2012 at 22:18, Adam Prescott wrote:

Assignment always returns the right hand side, so this isn't quite an
accurate demonstration of #store.

Although, I should also mention, this is due to the syntax of assignment
here being also a method call, and not simply that #foo= will always return
the argument it's given.

Or, since it's easier shown than described, what I mean is:

X.new.send(:foo=, 10) #=> :nope

Updated by MartinBosslet (Martin Bosslet) almost 12 years ago

Right, good point. OK, then let me rephrase it for explicitly calling store. What I would prefer is:

h = { a: 1 }
h.store(:a, 2) # => 1
h.store(:b, 3) # => nil

That way I can check for collisions in one pass without having to call has_key? first. Otherwise
the result is evaluating the internal hash function twice, once for has_key? and once for the
following call to store.

Updated by cjheath (Clifford Heath) almost 12 years ago

On 29/03/2012, at 8:40 AM, MartinBosslet (Martin Bosslet) wrote:

Right, good point. OK, then let me rephrase it for explicitly calling store.
...
That way I can check for collisions in one pass without having to call has_key? first.

It's a nice thought, but about fifteen years too late - you can't just change
fundamental things like this without introducing all sorts of subtle bugs into
people's programs. Write a new method (e.g. put(x)) that has the behaviour
you want.

Clifford Heath.

Updated by MartinBosslet (Martin Bosslet) almost 12 years ago

cjheath (Clifford Heath) wrote:

On 29/03/2012, at 8:40 AM, MartinBosslet (Martin Bosslet) wrote:

Right, good point. OK, then let me rephrase it for explicitly calling store.
...
That way I can check for collisions in one pass without having to call has_key? first.

It's a nice thought, but about fifteen years too late - you can't just change
fundamental things like this without introducing all sorts of subtle bugs into
people's programs. Write a new method (e.g. put(x)) that has the behaviour
you want.

Doing so in one pass is only possible on the C level. No use in composing it in
Ruby, this still means that the hash function is evaluated twice.

Hash is probably way too popular for no one to rely on the current
return value, I agree. But put isn't taken yet - how about Hash#put with the described
behavior?

Updated by shyouhei (Shyouhei Urabe) almost 12 years ago

=begin

Hmm, here you are a patch (not tested though).

From c55a9c9fab30d51be77821bce36054fe365b49af Mon Sep 17 00:00:00 2001
Message-Id:
From: URABE, Shyouhei
Date: Thu, 29 Mar 2012 11:38:17 +0900
Subject: [PATCH 1/1] Hash#store to return former content [feature #6219]

Signed-off-by: URABE, Shyouhei

diff --git a/hash.c b/hash.c
index e9937ff..0cd4e43 100644
--- a/hash.c
+++ b/hash.c
@@ -1099,6 +1099,20 @@ copy_str_key(st_data_t str)
return (st_data_t)rb_str_new4((VALUE)str);
}

+struct hash_aset_tuple {

  • st_data_t old;
  • st_data_t new;
    +};

+static int
+hash_aset_i(st_data_t key, st_data_t *value, st_data_t arg)
+{

  • struct hash_aset_tuple *ptr = (void *)arg;
  • ptr->old = *value;
  • *value = ptr->new;
  • return ST_CONTINUE;
    +}

/*

  • call-seq:
  • hsh[key] = value        -> value
    

@@ -1120,15 +1134,19 @@ copy_str_key(st_data_t str)
VALUE
rb_hash_aset(VALUE hash, VALUE key, VALUE val)
{

  • struct hash_aset_tuple t;
    rb_hash_modify(hash);
    hash_update(hash, key);
  • t.new = val;
  • t.old = Qundef;
    if (RHASH(hash)->ntbl->type == &identhash || rb_obj_class(key) != rb_cString) {
  • st_insert(RHASH(hash)->ntbl, key, val);
  • st_update(RHASH(hash)->ntbl, key, hash_aset_i, (st_data_t)&t);
    }
    else {
  • st_insert2(RHASH(hash)->ntbl, key, val, copy_str_key);
  • st_data_t k = copy_str_key(key);
  • st_update(RHASH(hash)->ntbl, k, hash_aset_i, (st_data_t)&t);
    }
  • return val;
  • return t.old;
    }

static int

1.7.0.4

=end

Updated by MartinBosslet (Martin Bosslet) almost 12 years ago

shyouhei (Shyouhei Urabe) wrote:

=begin

Hmm, here you are a patch (not tested though).

Awesome, thank you Shyouhei. I'll be happy to test this tonight!

Updated by shyouhei (Shyouhei Urabe) almost 12 years ago

Note however, that nobu changed st_update API1 after I posted that. So it might not apply cleanly to the latest ruby trunk...

Updated by nobu (Nobuyoshi Nakada) almost 12 years ago

And, what should be returned if the key wasn't set?

Updated by rosenfeld (Rodrigo Rosenfeld Rosas) almost 12 years ago

nobu (Nobuyoshi Nakada) wrote:

And, what should be returned if the key wasn't set?

Its default value?

h = Hash.new {[]}
h.store('key', [1, 2]) # => []

Updated by MartinBosslet (Martin Bosslet) almost 12 years ago

rosenfeld (Rodrigo Rosenfeld Rosas) wrote:

nobu (Nobuyoshi Nakada) wrote:

And, what should be returned if the key wasn't set?

Its default value?

Seems reasonable to me. To detect whether a key hasn't been set yet,
one would check for the presence of the default value. That
doesn't catch cases where a key with a default value is actually
set, but that seems fine to me. Voices against this?

Updated by mame (Yusuke Endoh) almost 12 years ago

  • Status changed from Open to Assigned
  • Assignee set to matz (Yukihiro Matsumoto)
  • Target version set to 3.0

Hello,

I tentatively mark this as 3.0 issue because it changes the behavior.
But if matz accepts the change, 2.0 may be able to include it.

Personally, I'm neutral to the proposal itself.

--
Yusuke Endoh

Updated by nobu (Nobuyoshi Nakada) almost 12 years ago

rosenfeld (Rodrigo Rosenfeld Rosas) wrote:

And, what should be returned if the key wasn't set?

Its default value?

Also calling its default proc?

MartinBosslet (Martin Bosslet) wrote:

Seems reasonable to me. To detect whether a key hasn't been set yet,
one would check for the presence of the default value. That
doesn't catch cases where a key with a default value is actually
set, but that seems fine to me. Voices against this?

Calling default proc can be expensive. So I don't think it's good idea to call it always.
A patch to separate Hash#store from Hash#[]=, and let it return the old value.

Updated by rosenfeld (Rodrigo Rosenfeld Rosas) almost 12 years ago

How about hash.store(new_value, nil_value: :default)

This way, one could write "a = hash.store(1, nil_value: nil)", while the default :default would call the default proc if it exists.

Updated by rosenfeld (Rodrigo Rosenfeld Rosas) almost 12 years ago

Or Hash#store(new_value, return_nil_if_not_set: false)

Updated by MartinBosslet (Martin Bosslet) almost 12 years ago

nobu (Nobuyoshi Nakada) wrote:

Calling default proc can be expensive. So I don't think it's good idea to call it always.
A patch to separate Hash#store from Hash#[]=, and let it return the old value.

You are right, the default proc is a real "party pooper" here... Could something like Rodrigo
proposed work for the default proc case? Did you mean something like it when you proposed
separating Hash#store from #[]=, or did you think about something different?

Updated by matz (Yukihiro Matsumoto) almost 12 years ago

by spec, all assignment should return the assigning value (to enable assignment chain), so that I have to reject the original proposal to change the value from "a[k] = v".
but there's still room to change the return value of Hash#store.

Matz.

Updated by matz (Yukihiro Matsumoto) almost 12 years ago

  • Status changed from Assigned to Feedback

Updated by MartinBosslet (Martin Bosslet) almost 12 years ago

Yes, I neglected assignment in my initial proposal, silly idea :) Hash#store would be great, though! Apart from the problem of how to handle the default proc, are there still other reasons against this?

Actions #19

Updated by naruse (Yui NARUSE) over 3 years ago

  • Target version deleted (3.0)
Actions

Also available in: Atom PDF

Like0
Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0