Skip to content

Fix packrat caching with PagedSeqReader #65

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

Merged
merged 2 commits into from
Sep 8, 2015
Merged

Fix packrat caching with PagedSeqReader #65

merged 2 commits into from
Sep 8, 2015

Conversation

liskin
Copy link
Contributor

@liskin liskin commented Aug 4, 2015

This fixes #45.

The problem was that whenever PagedSeqReader is constructed, source is
assigned seq (although lazily) but since PagedSeq is not a subclass of
java.lang.CharSequence, an implicit conversion takes place via
Predef#SeqCharSequence, creating a new object. The problem is that this
happens every time PagedSeqReader#rest or drop is called, breaking packrat
caching entirely.

liskin added 2 commits August 4, 2015 16:12
This fixes #45.

The problem was that whenever PagedSeqReader is constructed, source is
assigned seq (although lazily) but since PagedSeq is not a subclass of
java.lang.CharSequence, an implicit conversion takes place via
Predef#SeqCharSequence, creating a new object. The problem is that this
happens every time PagedSeqReader#rest or drop is called, breaking packrat
caching entirely.
@gourlaysama
Copy link
Contributor

Hi @liskin, good catch, and sorry it took this long to get to it.

It is sad we can't get rid of PagedSeqReader entirely, anything related to PagedSeq seems to cause trouble at some point, but this works well. Thank you for contributing! 🎆

LGTM

gourlaysama added a commit that referenced this pull request Sep 8, 2015
Fix packrat caching with PagedSeqReader
@gourlaysama gourlaysama merged commit 895a882 into scala:1.0.x Sep 8, 2015
@liskin liskin deleted the pagedseqreader-packrat branch September 10, 2015 14:50
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.

2 participants