No problem, I managed to make the fixes to your patch, and found an implementation artifact that caused Enumerable#sort_by to fail the spec incorrectly. All fixes are made and pushed in cc9623c.
Feel free to check out that commit and the resolution of the bug for more information. And thanks for the help! On Sun, Aug 2, 2009 at 9:18 PM, Gerald Boersma<[email protected]> wrote: > Charlie: > > Ah. Makes sense. Thanks for taking a look. > > I can take another pass if you like. > > Cheers, > Gerald > > On 2009-08-02, at 5:20 PM, Charles Oliver Nutter <[email protected]> > wrote: > >> Ok, I think I see the problem. The new spec that's failing does an >> hash.each {} and inside the each does repeated hash.merge! calls. >> Previously, since we weren't raising errors for changes during >> iteration, this passed ok. But now, it fails, because each sees that >> the contents of the hash table are changing. >> >> I think that in your patch, the raise inside iteratorVisitAll may not >> be necessary. Since rehash itself checks if an iteration is in >> progress, that would fulfill the contract of the spec from JRUBY-3745 >> without breaking the new spec. >> >> I also noticed that your patch does iterator entry/exit without a >> try/finally pair. This could lead to a hash getting stuck in >> "iterating" mode if an error is raised during iteration, and never >> being rehashable. >> >> I'll make a few tweaks to your patch and see how it runs. >> >> On Sun, Aug 2, 2009 at 4:34 PM, Gerald Boersma<[email protected]> wrote: >>> >>> Charlie: >>> >>> Thanks. >>> >>> I was holding off on posting it there since it is not fully working. But >>> I >>> can put it there if you think that works better. >>> >>> Cheers, >>> Gerald >>> >>> On 2009-08-02, at 11:01 AM, Charles Oliver Nutter <[email protected]> >>> wrote: >>> >>>> Ok, I'll have a look at it. You can also just attach patches to the >>>> associated bugs in the future, so they're centrally tracked. >>>> >>>> - Charlie >>>> >>>> On Sat, Aug 1, 2009 at 3:14 AM, Gerald Boersma<[email protected]> >>>> wrote: >>>>> >>>>> Here's an updated patch that is closer to the Ruby implementation >>>>> approach. >>>>> Now passes the specific rspec tests, but fails on another: >>>>> >>>>> [java] Hash#merge! shouldn't raise spurious RuntimeErrors FAILED >>>>> [java] Expected to not get RuntimeError >>>>> [java] >>>>> >>>>> >>>>> /Users/gerald/dev/jruby/spec/mspec/lib/mspec/expectations/expectations.rb:15:in >>>>> `fail_with' >>>>> [java] >>>>> >>>>> /Users/gerald/dev/jruby/spec/mspec/lib/mspec/expectations/should.rb:19:in >>>>> `should_not' >>>>> [java] /Users/gerald/dev/jruby/spec/ruby/core/hash/merge_spec.rb:69 >>>>> >>>>> No idea on what is causing this one. Seems like a side effect of the >>>>> fix. >>>>> Any idea on what could cause this one? Any insight will be much >>>>> appreciated. >>>>> >>>>> In the meantime, I'll move on to another bug to expand my understanding >>>>> of >>>>> the JRuby core. >>>>> >>>>> Cheers, >>>>> Gerald >>>>> >>>>> >>>>> >>>>> >>>>> >>>>> On 31-Jul-09, at 11:44 AM, Gerald Boersma wrote: >>>>> >>>>>> ΩSo I have an approach, but I am not happy with it. Attached is a >>>>>> patch. >>>>>> >>>>>> - It does not work for Hash.sort_by. >>>>>> - It adds an instance variable to Hash, which doesn't smell right to >>>>>> me. >>>>>> >>>>>> Basically: >>>>>> - I set an instance variable on RubyHash which holds the block passed >>>>>> into >>>>>> each, each_pair, etc. >>>>>> - In rehash, I check the stack for the current thread to see if the >>>>>> iterator block appears. If so, I throw the appropriate exception. >>>>>> >>>>>> This works for all cases except sort_by, since it seems that the >>>>>> RubyHash.rehash is not even called in this case (?!?), and the >>>>>> implementation lives somewhere else (RubyEnumerable). >>>>>> >>>>>> I would prefer an approach similar to the Ruby C code, where the >>>>>> function >>>>>> hash_foreach_iter in hash.c is called from every context where the >>>>>> iterator >>>>>> is built, and it just compares an initial state to a final state of >>>>>> the >>>>>> hash. But I do not see a corresponding pattern in the Java code. >>>>>> >>>>>> Any suggestions welcome. >>>>>> >>>>>> Cheers, >>>>>> Gerald >>>>>> >>>>>> <rs3745.patch> >>>>>> >>>>>> >>>>>> >>>>>> On 31-Jul-09, at 8:33 AM, Charles Oliver Nutter wrote: >>>>>> >>>>>>> On Thu, Jul 30, 2009 at 7:29 PM, Gerald Boersma<[email protected]> >>>>>>> wrote: >>>>>>>> >>>>>>>> Where can I look in the code base to understand how to determine in >>>>>>>> what >>>>>>>> context the Hash.rehash method is called, i.e. if it is called from >>>>>>>> an >>>>>>>> iterator over the Hash or not? I see from the Hash.each method how >>>>>>>> the >>>>>>>> yield >>>>>>>> to the block is performed and how the reference to the block to be >>>>>>>> used >>>>>>>> is >>>>>>>> called. I am not sure how to use that to safely determine that a >>>>>>>> rehash >>>>>>>> to >>>>>>>> the same Hash is being called from that same block (or even if that >>>>>>>> is >>>>>>>> a >>>>>>>> good approach. It doesn't smell quite right to me...). >>>>>>>> >>>>>>>> Any suggestions? >>>>>>>> >>>>>>>> If you think this defect is too big a bite to start with (i.e. first >>>>>>>> requires deep understanding of JRuby architecture / design), any >>>>>>>> suggestions >>>>>>>> for easier ones are welcome. But I am willing to start anywhere... >>>>>>> >>>>>>> A quick look at RubyHash.rehash shows it to be pretty >>>>>>> straightforward. >>>>>>> I don't think this is too difficult to start with. Why don't you have >>>>>>> a look and feel free to post any questions back here. We're standing >>>>>>> by to help you help us :) >>>>>>> >>>>>>>> BTW, searching through the dev mail archive using CodeHaus is not >>>>>>>> working >>>>>>>> very well for me. Trying a search on "3745" or "RUBY-3745" gives me >>>>>>>> "Unfortunately, there was an unexpected issue processing your >>>>>>>> request. >>>>>>>> " >>>>>>>> (after I prove I am human... ;-) >>>>>>> >>>>>>> Yeah, we're pretty disappointed with the Codehaus mail archives. I >>>>>>> don't think they've ever worked. Try nabble or markmail or something. >>>>>>> >>>>>>> - Charlie >>>>>>> >>>>>>> --------------------------------------------------------------------- >>>>>>> To unsubscribe from this list, please visit: >>>>>>> >>>>>>> http://xircles.codehaus.org/manage_email >>>>>>> >>>>>>> >>>>>> >>>>> >>>>> >>>>> >>>>> --------------------------------------------------------------------- >>>>> To unsubscribe from this list, please visit: >>>>> >>>>> http://xircles.codehaus.org/manage_email >>>>> >>>>> >>>> >>>> --------------------------------------------------------------------- >>>> To unsubscribe from this list, please visit: >>>> >>>> http://xircles.codehaus.org/manage_email >>>> >>>> >>> >>> --------------------------------------------------------------------- >>> To unsubscribe from this list, please visit: >>> >>> http://xircles.codehaus.org/manage_email >>> >>> >>> >> >> --------------------------------------------------------------------- >> To unsubscribe from this list, please visit: >> >> http://xircles.codehaus.org/manage_email >> >> > > --------------------------------------------------------------------- > To unsubscribe from this list, please visit: > > http://xircles.codehaus.org/manage_email > > > --------------------------------------------------------------------- To unsubscribe from this list, please visit: http://xircles.codehaus.org/manage_email
