Where communities thrive


  • Join over 1.5M+ people
  • Join over 100K+ communities
  • Free without limits
  • Create your own community
People
Repo info
Activity
  • 12:48

    sjrd on v0.6.30

    (compare)

  • 11:42
    sjrd closed #3862
  • 11:42
    sjrd commented #3862
  • 11:42
    sjrd milestoned #3862
  • 11:42
    sjrd labeled #3862
  • 11:42
    sjrd opened #3862
  • 10:12
    sjrd edited #3808
  • 10:11
    sjrd edited #3778
  • 09:39
    Josef-Vonasek edited #3861
  • 09:37
    sjrd labeled #3861
  • 09:37
    sjrd closed #3861
  • 09:37
    sjrd commented #3861
  • 09:35
    Josef-Vonasek edited #3861
  • 09:33
    Josef-Vonasek edited #3861
  • 09:32
    Josef-Vonasek opened #3861
  • 09:29
    sjrd labeled #3798
  • 09:29
    sjrd labeled #3798
  • 05:10
    sjrd commented on d7d3421
  • Nov 18 17:57
    sjrd closed #3859
  • Nov 18 17:57

    sjrd on master

    Move runtime lib into the org.s… Fix outdated comments. Run the IR cleaner on linkerPri… and 1 more (compare)

Eric K Richardson
@ekrich
@sjrd I got the JS tests to work. Bothers me that 14 don't work in JVM. I suppose I should try and solve as many as possible rather than avoid?
Sébastien Doeraene
@sjrd
If they're about Ints and Strings, they're expected not to work, because the JVM makes no guarantees for the identity of boxed Integers and Strings (but Scala.js does guarantee things, so the tests are meaningful on JS). You should assume them away.
Eric K Richardson
@ekrich
So don't do the Cache thing as above either?
Sébastien Doeraene
@sjrd
No, don't do the cache thing for Ints and Strings.
Eric K Richardson
@ekrich
Ok, I will avoid them as you suggest. Also, with keySet tests with TestObj, if I cache the keys for lookup they can be found on JVM - bother me that that works on JS because I am not sure why.
s/bothers/bother
Sébastien Doeraene
@sjrd

Also, with keySet tests with TestObj, if I cache the keys for lookup they can be found on JVM - bother me that that works on JS because I am not sure why.

Hum, I'm not following. Could you elaborate?

Eric K Richardson
@ekrich
assertTrue(keySet.contains(testKey(1))) in this test.
@Test def testKeySetIsViewForQueriesWithCustomObjects(): Unit = {
    val mp = factory.empty[TestObj, TestObj]
    mp.put(testKey(1), TestObj(11))
It was TestObj(1) in the assert.
There is not a special view, it is in AbstractSet I think - let me check.
keySet() from AbstractMap
Do you want github links?
Sébastien Doeraene
@sjrd
keySet() from AbstractMap is implemented as a view of entrySet(). If the entrySet() is correct, then the inherited keySet() is correct.
It might not be the most efficient, though. But it will be correct.
Eric K Richardson
@ekrich
That is what I thought but not sure why that would work on JS but not JVM. Seems it shouldn't work on JS either. If it needs an IdentityBox to compare or am I not thinking correctly?
Sébastien Doeraene
@sjrd
Hum, I'm not sure I understand the issue. I'm probably too tired.
But if you don't understand why something works, try and write more tests to break it. This is the best way to write robust tests. Tests are not there to show that the implementation is correct. Tests are there to show that there's no way the implementation could be wrong.
Eric K Richardson
@ekrich
There are also those Double keys like 1.2345 that don't work on JVM. Should I try on straight JVM to see? It is super late there - maybe I do some tests and change a few things and you can inspect later to see if you agree - I think I have a better idea how to proceed at least in some cases.
Is it easy enough to remove commits out of a sequence if you don't like it? Haven't done that before.
Sébastien Doeraene
@sjrd
Doubles won't be cached on the JVM, so they'll have different identities. Same thing: assume those tests away.
Eric K Richardson
@ekrich
Ok, thanks. I'll do those first.
Sébastien Doeraene
@sjrd

Is it easy enough to remove commits out of a sequence if you don't like it? Haven't done that before.

Yes, it's trivial with git rebase --interactive (you should learn how to use that)

Eric K Richardson
@ekrich
ok, super - just haven't learned that yet but this may be the place to do it.
Have a good evening.
Sébastien Doeraene
@sjrd
Thanks. Same to you.
(or whatever applies in your timezone ^^)
Eric K Richardson
@ekrich
Yeah, west coast US so only 3PM so far.
Eric K Richardson
@ekrich
@sjrd Good news, I needed a keySet view and adding it gave me errors on the JS side I was seeing on the JVM side. So that solves some of the probelms.
Eric K Richardson
@ekrich
This test in JVM assertTrue(entrySet.remove(SIE(testKey(1), TestObj(11)))) appears to be a bug. in MapTest @Test def testEntrySetIsViewForRemoveWithCustomObjects(): Unit = {
Eric K Richardson
@ekrich
@sjrd I am needing to override removeAll and retainAll and was wondering what this optimization or feature is doing exactly. https://github.com/scala-js/scala-js/blob/0.6.x/javalib/src/main/scala/java/util/AbstractSet.scala#L35-L39
removeAll for keySet seems to work ok with just the first part.
Sébastien Doeraene
@sjrd
That optimization is questionable, but it is specified in the JavaDoc. So you'll find your answer in the JavaDoc.
The idea is to iterate on the smaller of the two sets.
Eric K Richardson
@ekrich
Ok, thank you. That is understandable.
Eric K Richardson
@ekrich
@sjrd I still have the new tests to add but I think that'll have to wait for tomorrow. I did address all the other things so if you see anything I missed or misinterpreted, please let me know. Thanks for being patient - this was quite a bit harder and more involved than I expected.
Sébastien Doeraene
@sjrd
It looks great :) I didn't see anything. So now it's just the few additional tests and it will be good to go. I'm looking forward to merge this PR. It's a great addition to our library.
Eric K Richardson
@ekrich
Thanks for the kind words. I'll let you know when I complete the tests.
Tobias Schlatter
@gzm0
@sjrd It seems like I'm soon done with loading the linker reflectively. Is there anything I can do to speed up 1.0? Maybe take over the RuntimeLong PR and then do the same for UndefinedBehaviorError?
Sébastien Doeraene
@sjrd
Yes, the RuntimeLong PR is a good idea. It should straightforward once rebased on the loading-the-linker-reflectively PR.
For UndefinedBehaviorError I don't even know what to do about it. It needs to be accessible to user code (e.g., testing frameworks might want to catch them), so we can't just hide it as an implementation detail of the linker.
I am almost done with the refactoring of names. Cleaning up the history at this point.
Tobias Schlatter
@gzm0
Why can't we hide it? It should extend Throwable for sure, but then testing frameworks can catch it. Do you think they need to special case it?
Sébastien Doeraene
@sjrd
I don't know. Probably not.
Searching "UndefinedBehaviorError" doesn't seem to return any code result besides our own repo, so maybe it's fine to hide it after all.
(searching on Google)
Tobias Schlatter
@gzm0
FYI I accidentally pushed to master directly. I have force-pushed backwards by disabling branch protection temporarily.
Tobias Schlatter
@gzm0
@sjrd FYI I will not have time to work on anything until Tuesday evening. Feel free to hijack anything you like :)
Sébastien Doeraene
@sjrd
OK thanks :)
I'm taking care of #3838 atm.
Tobias Schlatter
@gzm0
Ack