Clojure

Empty transient maps/sets return wrong value for .contains

Details

  • Type: Defect Defect
  • Status: Closed Closed
  • Priority: Major Major
  • Resolution: Completed
  • Affects Version/s: None
  • Fix Version/s: Release 1.5
  • Component/s: None
  • Labels:
    None
  • Patch:
    Code and Test
  • Approval:
    Ok

Description

As explained in http://groups.google.com/group/clojure/browse_thread/thread/496ffc7fb9058699/6e9d8152b6fd6365?show_docid=6e9d8152b6fd6365, (.contains (transient #{}) :some-key) returns true instead of false. (contains? #{} :some-key) works properly, but the plain .contains method does not.

The issue is in TransientHashMap.doValAt(key, notFound), and the attached patch is a simple fix with test.

Activity

Hide
Stuart Halloway added a comment -

IIRC there are broader issues around lookup from transients. Should we look at this patch alone, or step back and review?

Show
Stuart Halloway added a comment - IIRC there are broader issues around lookup from transients. Should we look at this patch alone, or step back and review?
Hide
Rich Hickey added a comment -

take the patch if it fixes the problem

Show
Rich Hickey added a comment - take the patch if it fixes the problem
Hide
Andy Fingerhut added a comment -

clj-757-fix-behavior-of-empty-transient-maps-patch2.txt is an update of Alan's original patch, with no changes other than those required to apply cleanly to latest master as of Feb 22, 2012. Compiles fine and runs without test errors, and does fix the behavior of the one new unit test included.

Show
Andy Fingerhut added a comment - clj-757-fix-behavior-of-empty-transient-maps-patch2.txt is an update of Alan's original patch, with no changes other than those required to apply cleanly to latest master as of Feb 22, 2012. Compiles fine and runs without test errors, and does fix the behavior of the one new unit test included.
Hide
Andy Fingerhut added a comment -

Approval was changed from Test to Incomplete when it was waiting on Rich's feedback. He did give his feedback, but Approval never changed back to Test. Doing that now in hopes it makes the ticket more accurately categorized by filters. Feel free to give me a stern warning for mucking with the Approval field if I'm out of line here.

Show
Andy Fingerhut added a comment - Approval was changed from Test to Incomplete when it was waiting on Rich's feedback. He did give his feedback, but Approval never changed back to Test. Doing that now in hopes it makes the ticket more accurately categorized by filters. Feel free to give me a stern warning for mucking with the Approval field if I'm out of line here.
Hide
Andy Fingerhut added a comment -

clj-757-fix-behavior-of-empty-transient-maps-patch3.txt dated June 18, 2012 is identical to clj-757-fix-behavior-of-empty-transient-maps-patch2.txt, except it has some updated context lines so that it applies cleanly with latest Clojure master. I will remove the -patch2.txt patch since it no longer applies cleanly.

Show
Andy Fingerhut added a comment - clj-757-fix-behavior-of-empty-transient-maps-patch3.txt dated June 18, 2012 is identical to clj-757-fix-behavior-of-empty-transient-maps-patch2.txt, except it has some updated context lines so that it applies cleanly with latest Clojure master. I will remove the -patch2.txt patch since it no longer applies cleanly.
Hide
Fogus added a comment -

The patch in clj-757-fix-behavior-of-empty-transient-maps-patch3.txt is straight-forward and so is its test. This fixes only the behavior listed in the ticket and does not address the the general transient lookup behavior.

Show
Fogus added a comment - The patch in clj-757-fix-behavior-of-empty-transient-maps-patch3.txt is straight-forward and so is its test. This fixes only the behavior listed in the ticket and does not address the the general transient lookup behavior.

People

Vote (0)
Watch (2)

Dates

  • Created:
    Updated:
    Resolved: