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.

Sign up for GitHub

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

Jump to bottom

Remove scala.Phantom (replaced by unused) #3410

Merged
merged 2 commits into from
Mar 1, 2018
Merged

Remove scala.Phantom (replaced by unused) #3410

merged 2 commits into from
Mar 1, 2018

Conversation

nicolasstucki
Copy link
Contributor

No description provided.

@nicolasstucki nicolasstucki mentioned this pull request Oct 31, 2017
Merged
14 tasks
@nicolasstucki nicolasstucki force-pushed the make-phantoms-unused branch 3 times, most recently from 9df33c1 to 391faf7 Compare October 31, 2017 14:31
@nicolasstucki
Copy link
Contributor Author

Rebased on #3342

This was referenced Jan 9, 2018
Closed
Closed
@nicolasstucki nicolasstucki self-assigned this Jan 18, 2018
@nicolasstucki
Make all phantoms unused
b567b8d 
This removes the necesity of erasing phantoms. This process
is done by the erasure of `unused`.
@nicolasstucki
Copy link
Contributor Author

test performance please

@dottybot
Copy link
Member

performance test scheduled: 1 job(s) in queue, 0 running.

@dottybot
Copy link
Member

performance test failed:

Error line number: 24

[check /data/workspace/bench/logs/pull-3410-02-22-07.23.out for more information]

@nicolasstucki
Copy link
Contributor Author

@liufengyun something is wrong with the test performance please from a PR

@liufengyun
Copy link
Contributor

@nicolasstucki The merge of unused causes a regression of scalapb project in the benchmarks.

@liufengyun
Copy link
Contributor

BTW @nicolasstucki , try log into lamppc37 and you should be able to read that log file.

@nicolasstucki
Copy link
Contributor Author

There is some duplicated work done for phantoms an unused that is present in the last merge. This PR removes it. Hopefully that slowdown is because of this duplication of checks.

@liufengyun
Copy link
Contributor

Sorry, I wasn't clear, the scalapb project doesn't compile after the merge.

@liufengyun
Copy link
Contributor

The community build has the same error message: https://travis-ci.org/lampepfl/dotty-community-build/jobs/344252455

@nicolasstucki nicolasstucki changed the title Make phantoms unused Remove scala.Phantom (replaced by unused) Feb 25, 2018
@nicolasstucki nicolasstucki mentioned this pull request Feb 25, 2018
Closed
@nicolasstucki
Copy link
Contributor Author

test performance please

@dottybot
Copy link
Member

performance test scheduled: 1 job(s) in queue, 0 running.

@dottybot
Copy link
Member

Performance test finished successfully:

Visit http://dotty-bench.epfl.ch/3410/ to see the changes.

Benchmarks is based on merging with master (01f3094)

@nicolasstucki nicolasstucki removed their assignment Feb 26, 2018
odersky
odersky approved these changes Mar 1, 2018
View reviewed changes
Copy link
Contributor

@odersky odersky left a comment

Choose a reason for hiding this comment

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

LGTM. I think it's good to remove this now to make clear where we are. The PR is very useful when read in reverse as a record what we should add, in hopefully simpler form, to support effect types.

@odersky odersky merged commit 4d41e4f into scala:master Mar 1, 2018
@allanrenucci allanrenucci deleted the make-phantoms-unused branch March 1, 2018 10:57
@nicolasstucki nicolasstucki mentioned this pull request Mar 1, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@odersky odersky odersky approved these changes

Assignees

@odersky odersky

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@nicolasstucki @dottybot @liufengyun @odersky