Skip to content
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.

Commit 8198bd5

Browse files
headiusheadius
committedJan 25, 2018
Make RubyHash#put return existing value, as per Map contract.
Fixes #4916.
1 parent 2cb38f3 commit 8198bd5

File tree

3 files changed

+54
-14
lines changed

3 files changed

+54
-14
lines changed
 

‎core/src/main/java/org/jruby/RubyHash.java

+24-11
Original file line numberDiff line numberDiff line change
@@ -502,8 +502,6 @@ protected final void checkIterating() {
502502
throw getRuntime().newRuntimeError("can't add a new key into hash during iteration");
503503
}
504504
}
505-
// ------------------------------
506-
public static long collisions = 0;
507505

508506
// put implementation
509507

@@ -512,26 +510,32 @@ private final void internalPut(final IRubyObject key, final IRubyObject value) {
512510
}
513511

514512
private final void internalPutSmall(final IRubyObject key, final IRubyObject value) {
515-
internalPutSmall(key, value, true);
513+
internalPutNoResize(key, value, true);
516514
}
517515

518516
protected void internalPut(final IRubyObject key, final IRubyObject value, final boolean checkForExisting) {
519517
checkResize();
520518

521-
internalPutSmall(key, value, checkForExisting);
519+
internalPutNoResize(key, value, checkForExisting);
522520
}
523521

524-
protected void internalPutSmall(final IRubyObject key, final IRubyObject value, final boolean checkForExisting) {
522+
protected final IRubyObject internalJavaPut(final IRubyObject key, final IRubyObject value) {
523+
checkResize();
524+
525+
return internalPutNoResize(key, value, true);
526+
}
527+
528+
protected IRubyObject internalPutNoResize(final IRubyObject key, final IRubyObject value, final boolean checkForExisting) {
525529
final int hash = hashValue(key);
526530
final int i = bucketIndex(hash, table.length);
527531

528-
// if (table[i] != null) collisions++;
529-
530532
if (checkForExisting) {
531533
for (RubyHashEntry entry = table[i]; entry != null; entry = entry.next) {
532534
if (internalKeyExist(entry, hash, key)) {
535+
IRubyObject existing = entry.value;
533536
entry.value = value;
534-
return;
537+
538+
return existing;
535539
}
536540
}
537541
}
@@ -540,6 +544,9 @@ protected void internalPutSmall(final IRubyObject key, final IRubyObject value,
540544

541545
table[i] = new RubyHashEntry(hash, key, value, table[i], head);
542546
size++;
547+
548+
// no existing entry
549+
return null;
543550
}
544551

545552
// get implementation
@@ -1003,7 +1010,7 @@ protected void op_asetSmallForString(Ruby runtime, RubyString key, IRubyObject v
10031010
} else {
10041011
checkIterating();
10051012
if (!key.isFrozen()) key = (RubyString)key.dupFrozen();
1006-
internalPutSmall(key, value, false);
1013+
internalPutNoResize(key, value, false);
10071014
}
10081015
}
10091016

@@ -2049,8 +2056,9 @@ public Object get(Object key) {
20492056

20502057
@Override
20512058
public Object put(Object key, Object value) {
2052-
internalPut(JavaUtil.convertJavaToUsableRubyObject(getRuntime(), key), JavaUtil.convertJavaToUsableRubyObject(getRuntime(), value));
2053-
return value;
2059+
Ruby runtime = getRuntime();
2060+
IRubyObject existing = internalJavaPut(JavaUtil.convertJavaToUsableRubyObject(runtime, key), JavaUtil.convertJavaToUsableRubyObject(runtime, value));
2061+
return existing == null ? null : existing.toJava(Object.class);
20542062
}
20552063

20562064
@Override
@@ -2490,4 +2498,9 @@ public IRubyObject default_value_get(ThreadContext context, IRubyObject[] args)
24902498
throw context.runtime.newArgumentError(args.length, 1);
24912499
}
24922500
}
2501+
2502+
@Deprecated
2503+
protected void internalPutSmall(final IRubyObject key, final IRubyObject value, final boolean checkForExisting) {
2504+
internalPutNoResize(key, value, checkForExisting);
2505+
}
24932506
}

‎core/src/main/java/org/jruby/java/proxies/MapJavaProxy.java

+11-3
Original file line numberDiff line numberDiff line change
@@ -178,15 +178,23 @@ public RubyHash delete_ifInternal(final ThreadContext context, final Block block
178178

179179
@Override
180180
public void internalPut(final IRubyObject key, final IRubyObject value, final boolean checkForExisting) {
181-
internalPutSmall(key, value, checkForExisting);
181+
internalPutNoResize(key, value, checkForExisting);
182182
}
183183

184184
@Override
185-
protected final void internalPutSmall(IRubyObject key, IRubyObject value, boolean checkForExisting) {
185+
protected final IRubyObject internalPutNoResize(IRubyObject key, IRubyObject value, boolean checkForExisting) {
186186
@SuppressWarnings("unchecked")
187+
Ruby runtime = getRuntime();
187188
final Map<Object, Object> map = mapDelegate();
188-
map.put(key.toJava(Object.class), value.toJava(Object.class));
189+
Object javaValue = value.toJava(Object.class);
190+
Object existing = map.put(key.toJava(Object.class), javaValue);
189191
setSize( map.size() );
192+
if (existing != null) {
193+
if (existing == javaValue) return value;
194+
return JavaUtil.convertJavaToUsableRubyObject(runtime, existing);
195+
}
196+
// none existing
197+
return null;
190198
}
191199

192200
@Override

‎core/src/test/java/org/jruby/test/TestRubyHash.java

+19
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,7 @@
3636
import org.jruby.RubyHash;
3737
import org.jruby.RubySymbol;
3838
import org.jruby.exceptions.RaiseException;
39+
import org.jruby.runtime.builtin.IRubyObject;
3940

4041
/**
4142
* @author chadfowler
@@ -194,4 +195,22 @@ public void testGet() {
194195
public void testDoubleQuotedUtf8HashKey() throws Exception {
195196
assertEquals("UTF-8", eval("# encoding: utf-8\n h = { \"Ãa1\": true }\n puts h.keys.first.encoding"));
196197
}
198+
199+
public void testPutExisting() {
200+
Ruby ruby = Ruby.newInstance();
201+
IRubyObject hash = ruby.evalScriptlet("{ \"Jane Doe\" => 10, \"Jim Doe\" => 6 }");
202+
assertTrue(RubyHash.class.isInstance(hash));
203+
RubyHash rubyHash = (RubyHash) hash;
204+
// Should return previous value 10, but returns the new value 42
205+
assertEquals(10L, rubyHash.put("Jane Doe", 42));
206+
}
207+
208+
public void testPutNotExisting() {
209+
Ruby ruby = Ruby.newInstance();
210+
IRubyObject hash = ruby.evalScriptlet("{ \"Jim Doe\" => 6 }");
211+
assertTrue(RubyHash.class.isInstance(hash));
212+
RubyHash rubyHash = (RubyHash) hash;
213+
// Should return null because the key wasn't present before, but returns the new value 42
214+
assertEquals(null, rubyHash.put("Jane Doe", 42));
215+
}
197216
}

0 commit comments

Comments
 (0)
Please sign in to comment.