Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement j.u.Collection.removeIf. #2013

Conversation

LeeTibbert
Copy link
Contributor

@LeeTibbert LeeTibbert commented Nov 16, 2020

This PR builds upon merged PRs #1937 & #2040 by re-porting Collection.scala and
AbstractCollection.scala from Scala.js.

The corresponding tests and TrivialImmutableCollection.scala were freshly ported from Scala.js by
Lorenzo G (lolgab) as part of his PR #1994 and are used by his kind permission.

Collection.scala provides the default methods. AbstractCollectionTest provides a concrete
factory type which exercises the methods of both Collection.scala & AbstractCollection.scala.

LeeTibbert added a commit to LeeTibbert/scala-native-fork that referenced this pull request Nov 16, 2020
The test was ported from Scala.js by Lorenzo G (lolgab) as
part of his PR scala-native#1994 and used with his kind permission.

This PR is WIP because it will fail to build until after
PR scala-native#2013 (Collection) and its series of prerequisites are merged
and this PR re-based.
LeeTibbert added a commit to LeeTibbert/scala-native-fork that referenced this pull request Nov 16, 2020
The test was ported from Scala.js by Lorenzo G (lolgab) as
part of his PR scala-native#1994 and used with his kind permission.

This PR is WIP because it will fail to build until after
PR scala-native#2013 (Collection) and its series of prerequisites are merged
and this PR re-based.
@LeeTibbert LeeTibbert force-pushed the PR_j_u_CollectionTraitDefaultMethods_2020-11-16 branch from 1e6aaf0 to 45c517d Compare November 17, 2020 17:07
Comment on lines 205 to 220
// Issue: https://github.com/scala-native/scala-native/issues/1972
@Ignore(
"removeIf is a JavaDefaultMethod which is not supported yet on Scala 2.11")
@Test def removeIf(): Unit = {
// val coll = factory.fromElements[Int](42, 50, 12, 0, -45, 102, 32, 75)
// assertEquals(8, coll.size())

// assertTrue(coll.removeIf(new java.util.function.Predicate[Int] {
// def test(x: Int): Boolean = x >= 50
// }))
// assertEquals(5, coll.size())
// assertIteratorSameElementsAsSet(-45, 0, 12, 32, 42)(coll.iterator())

// assertFalse(coll.removeIf(new java.util.function.Predicate[Int] {
// def test(x: Int): Boolean = x >= 45
// }))
// assertEquals(5, coll.size())
// assertIteratorSameElementsAsSet(-45, 0, 12, 32, 42)(coll.iterator())
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you can now uncomment this test and remove the // Issue: ... comment I added and the @Ignore annotation. Take the original file as guide.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops! Thank you for catching that. It is kind of one of the main motivators for the Test.
I had read your documentation about the ignore and seen the code, but the message
never arrived to my fingers when I copied the code. Fix in process.

@sjrd
Copy link
Collaborator

sjrd commented Nov 18, 2020

Is this still WiP?

@LeeTibbert
Copy link
Contributor Author

Thank you for looking at this.

Yes, it is still WIP. For final testing, it depends upon the Iterable PR #2009 which
is passing Travis and ready for review.

This PR is passing, and I have tested using my private implementation of Iterable,
but I would like to see it pass after PR #2009 is merged before I promote it out of
WIP. My hope was to save and help focus reviewer time.

@sjrd
Copy link
Collaborator

sjrd commented Nov 18, 2020

I merged #2009.

@LeeTibbert LeeTibbert force-pushed the PR_j_u_CollectionTraitDefaultMethods_2020-11-16 branch from b7aa231 to 4080457 Compare November 18, 2020 19:59
@LeeTibbert
Copy link
Contributor Author

LeeTibbert commented Nov 18, 2020

Sjrd. Thank you for the rapid merge of PR #2009

@LeeTibbert LeeTibbert changed the title WIP: Annotate & test j.u.Collection default methods Annotate & test j.u.Collection default methods Nov 18, 2020
@LeeTibbert
Copy link
Contributor Author

LeeTibbert commented Nov 19, 2020

PR #2022 reverts the Iterable PR #2009 whilst I chase the underling issue.
So I am moving this PR back to WIP, to keep things properly sequenced.

@LeeTibbert LeeTibbert changed the title Annotate & test j.u.Collection default methods WIP: Annotate & test j.u.Collection default methods Nov 19, 2020
@LeeTibbert LeeTibbert force-pushed the PR_j_u_CollectionTraitDefaultMethods_2020-11-16 branch from 4080457 to 5f6bd8f Compare November 28, 2020 16:55
@LeeTibbert
Copy link
Contributor Author

Re-based & re-built now that PR #2040 has been merged. CI is all green.
There are no "can't call" warnings/errors in the one Scala 2.11 log file
I checked: (2.11.12, release-fast, commix).

@LeeTibbert LeeTibbert changed the title WIP: Annotate & test j.u.Collection default methods Annotate & test j.u.Collection default methods Nov 28, 2020
import scala.scalanative.junit.utils.AssertThrows._
import scala.scalanative.junit.utils.Utils._

trait CollectionTest extends IterableTest {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file on its own is all dead code. We would need at least one concrete subclass of CollectionTest with a concrete factory type to make this non dead code.

So either CollectionTest and TrivialImmutableCollection should be removed from this PR (and instead introduced in a PR that actually has a least one concrete subclass), or we should add such a concrete subclass in this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I chose the second alternative and moved AbstractCollection.scala & AbstractCollectionTest.scala
from PR #2014. This keeps the two sets of file.scala & fileTest.scala in the same PR.
AbstractCollectionTest implements the concrete subclass.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PS: thank you for clearly describing the alternatives.

This PR builds upon merged PR scala-native#1937 by porting Collection.scala and its
corresponding test from Scala.js.

The test was ported from Scala.js by Lorenzo G (lolgab) as
part of his PR scala-native#1994.

See Issue scala-native#1972 for background & discussion of JavaDefaultMethod annotation.

This PR is WIP because it will fail to build until after
PR scala-native#1997 (nscplugin), scala-native#2009 (j.l.Iterable), & 2010 (TrivialCollection)
are merged and this PR is re-based.
@LeeTibbert LeeTibbert changed the title Annotate & test j.u.Collection default methods Annotate j.u.Collection default methods Nov 29, 2020
@LeeTibbert LeeTibbert force-pushed the PR_j_u_CollectionTraitDefaultMethods_2020-11-16 branch from 5f6bd8f to e72e2ff Compare November 29, 2020 23:16
@LeeTibbert LeeTibbert requested a review from sjrd November 30, 2020 00:04
@sjrd sjrd changed the title Annotate j.u.Collection default methods Implement j.u.Collection.removeIf. Nov 30, 2020
@sjrd sjrd merged commit a3e9e7f into scala-native:master Nov 30, 2020
ekrich pushed a commit to ekrich/scala-native that referenced this pull request May 21, 2021
We test it through `AbstractCollection`, which we update as well.

Ported from Scala.js.
WojciechMazur pushed a commit to WojciechMazur/scala-native that referenced this pull request Aug 25, 2021
We test it through `AbstractCollection`, which we update as well.

Ported from Scala.js.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants