RVM

Consolidate EmptyIterator implementations

Details

  • Type: Improvement Improvement
  • Status: Closed Closed
  • Priority: Minor Minor
  • Resolution: Fixed
  • Affects Version/s: 3.1.2
  • Fix Version/s: 3.1.3
  • Component/s: Compiler: Optimizing
  • Labels:
    None
  • Patch Submitted:
    Yes
  • Number of attachments :
    2

Description

Currently, Jikes RVM comes with two implementations of an "empty iterator": org.jikesrvm.util.EmptyIterator and org.jikesrvm.compilers.opt.util.EmptyIterator. Both implementations have shortcomings (no singleton iterator and no generics, respectively) and don't throw the exceptions expected of the Java 7 java.util.Collections.emptyIterator http://docs.oracle.com/javase/7/docs/api/java/util/Collections.html#emptyIterator%28%29.

The attached patch consolidates both implementations into a single, generic org.jikesrvm.util.EmptyIterator.

  1. Text File
    consolidate-empty-iterators.patch
    01/May/12 11:11 AM
    3 kB
    Andreas Sewe
  2. File
    RVM-965-updated.diff
    04/Aug/12 3:25 PM
    8 kB
    Erik Brangs

Activity

Ascending order - Click to sort in descending order
David Grove made changes -
Field Original Value New Value
Assignee David Grove [ dgrove ]
Hide
Permalink
David Grove added a comment -

Just realized I never reported this. I tried applying the patch in May. I didn't commit it then because the modofied system failed one of the precommit tests. I didn't dig too deeply, but I suspect that some piece of the opt compiler maybe was relying on the old behavior on empty iteration.

Show
David Grove added a comment - Just realized I never reported this. I tried applying the patch in May. I didn't commit it then because the modofied system failed one of the precommit tests. I didn't dig too deeply, but I suspect that some piece of the opt compiler maybe was relying on the old behavior on empty iteration.
Hide
Permalink
Erik Brangs added a comment -

A cursory look at the source code did not yield any obvious candidates for a possible problem and I can't reproduce the failed pre-commit tests:

  1. The revision number from the patch is not from the main repo, so I tried applying the patch to to revision f8e2312d9c1a. For this revision, the patch applies cleanly and the pre-commit tests on x86_64-linux do not fail.
  2. I also tested an updated (and slightly improved) version of the patch on hg tip. That one does not fail either. However, I don't see any point in attaching it until the issue with pre-commit is cleared up.
Show
Erik Brangs added a comment - A cursory look at the source code did not yield any obvious candidates for a possible problem and I can't reproduce the failed pre-commit tests: The revision number from the patch is not from the main repo, so I tried applying the patch to to revision f8e2312d9c1a . For this revision, the patch applies cleanly and the pre-commit tests on x86_64-linux do not fail. I also tested an updated (and slightly improved) version of the patch on hg tip . That one does not fail either. However, I don't see any point in attaching it until the issue with pre-commit is cleared up.
Hide
Permalink
David Grove added a comment -

it's always possible that the pre-commit failure I saw before was because I botched the patch (I didn't apply cleanly, so I adjusted it manually; maybe I screwed that up). Attach the new patch and we'll try again?

Show
David Grove added a comment - it's always possible that the pre-commit failure I saw before was because I botched the patch (I didn't apply cleanly, so I adjusted it manually; maybe I screwed that up). Attach the new patch and we'll try again?
Hide
Permalink
Erik Brangs added a comment -

I've attached the updated version of Andreas Sewe's patch.

Show
Erik Brangs added a comment - I've attached the updated version of Andreas Sewe's patch.
Erik Brangs made changes -
Attachment RVM-965-updated.diff [ 60816 ]
Hide
Permalink
David Grove added a comment -

I applied the updated patch in 10472:0300e76993fd. pre-commit passed 3 times in a row, so we'll attribute the failures I was seeing before to Dave having a bad day and not doing the merge of the patch to the tip correctly...

Show
David Grove added a comment - I applied the updated patch in 10472:0300e76993fd. pre-commit passed 3 times in a row, so we'll attribute the failures I was seeing before to Dave having a bad day and not doing the merge of the patch to the tip correctly...
David Grove made changes -
Status Open [ 1 ] Resolved [ 5 ]
Fix Version/s 3.1.3 [ 18230 ]
Resolution Fixed [ 1 ]
Hide
Permalink
Andreas Sewe added a comment -

Thank you, Dave and Erik, for taking care of this.

Show
Andreas Sewe added a comment - Thank you, Dave and Erik, for taking care of this.
Hide
Permalink
David Grove added a comment -

bulk close of all resolved issues in preparation for 3.1.3 release.

Show
David Grove added a comment - bulk close of all resolved issues in preparation for 3.1.3 release.
David Grove made changes -
Status Resolved [ 5 ] Closed [ 6 ]

People

  • Assignee:
    David Grove
    Reporter:
    Andreas Sewe
Vote (1)
Watch (2)

Dates

  • Created:
    Updated:
    Resolved: