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

Basic implementation of Marshal.load and Marshal.dump. #1191

Merged
merged 2 commits into from Mar 31, 2016

Conversation

Projects
None yet
4 participants
@iliabylich
Member

iliabylich commented Nov 17, 2015

Things that has been changed to make Marshal working:

  • Array#instance_variables doesn't return indexes
  • Module#instance_variables doesn't return constants
  • String subclasses do not return @literal in ivars list.

Limitations:

  • Marshal.dump(:symbol) - no Symbols support
  • String encoding - dumping/loading a string with custom encoding always dumps/loads utf-16 encoded string
  • Everything about String mutating - extended strings, ivars on strings and so on.
  • Bignum's can't be encoded/decoded

218 examples from rubyspec are passing (Marshal#restore is an alias to Marshal#load, so probably even 300), ~50 are failing.

@iliabylich iliabylich force-pushed the iliabylich:marshal-dump-load-restore-implementation-attempt branch 2 times, most recently from 8a69010 to 8f92442 Nov 19, 2015

@iliabylich iliabylich force-pushed the iliabylich:marshal-dump-load-restore-implementation-attempt branch from 8f92442 to c87832e Jan 6, 2016

@iliabylich iliabylich force-pushed the iliabylich:marshal-dump-load-restore-implementation-attempt branch from 81f5352 to 0414c54 Jan 27, 2016

@iliabylich iliabylich force-pushed the iliabylich:marshal-dump-load-restore-implementation-attempt branch from 0414c54 to b7f74bc Feb 8, 2016

@iliabylich

This comment has been minimized.

Member

iliabylich commented Mar 21, 2016

@elia I know in MRI Marshal is a part of corelib. What do you think about moving it to stdlib for Opal? Then it will not change the size of opal/base or opal/mini

@elia

This comment has been minimized.

Member

elia commented Mar 22, 2016

@iliabylich this what I was thinking, let me know your considerations, I was pondering whether to introduce an opal/full and slim down the default opal including in it the things that are useful in the browser for most of the time, that way we can both 1. keep its size in check and 2. freely implement cool stuff like marshalling without fearing for the size (it's just a require "corelib/THING" away for those ho need it).

Just as a note I'll add that MRuby goes even further including in its basic bundle a dumbed down version of many methods (e.g. gsub can't accept a block and stuff like that). I think in our case it would make the core lib more fragile, but it's still a solution to keep in mind.

cc @meh

eregon added a commit to ruby/mspec that referenced this pull request Mar 31, 2016

@eregon

This comment has been minimized.

Contributor

eregon commented Mar 31, 2016

FWIW, I fixed mspec upstream in ruby/mspec@203ec67.

@iliabylich

This comment has been minimized.

Member

iliabylich commented Mar 31, 2016

@eregon Thank you so much! I was going to open a PR to mspec but for some reason I thought that it will not be be accepted 😄

@eregon

This comment has been minimized.

Contributor

eregon commented Mar 31, 2016

@iliabylich @elia @vais Anything that makes mspec simpler is good and a welcome change!

@iliabylich

This comment has been minimized.

Member

iliabylich commented Mar 31, 2016

@eregon Thanks, pulling udpates!
@elia I'll try to rebase it to the latest master (if it's possible 😄). Just a final question, are you ok with excluding marshal stuff from opal/opal and forcing everyine to require 'corelib/marshal' manually?

@elia

This comment has been minimized.

Member

elia commented Mar 31, 2016

@iliabylich I am, let's also add an opal/full

@iliabylich iliabylich force-pushed the iliabylich:marshal-dump-load-restore-implementation-attempt branch from b7f74bc to 8c05d1d Mar 31, 2016

@coveralls

This comment has been minimized.

coveralls commented Mar 31, 2016

Coverage Status

Coverage decreased (-0.09%) to 87.583% when pulling 8c05d1d on iliabylich:marshal-dump-load-restore-implementation-attempt into 3ee438a on opal:master.

@iliabylich iliabylich force-pushed the iliabylich:marshal-dump-load-restore-implementation-attempt branch from 8c05d1d to ba9f740 Mar 31, 2016

@coveralls

This comment has been minimized.

coveralls commented Mar 31, 2016

Coverage Status

Coverage decreased (-0.09%) to 87.583% when pulling ba9f740 on iliabylich:marshal-dump-load-restore-implementation-attempt into 3ee438a on opal:master.

@iliabylich

This comment has been minimized.

Member

iliabylich commented Mar 31, 2016

Done

@elia elia merged commit 9236b4d into opal:master Mar 31, 2016

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.09%) to 87.583%
Details
@elia

This comment has been minimized.

Member

elia commented Mar 31, 2016

w00t!

@iliabylich iliabylich deleted the iliabylich:marshal-dump-load-restore-implementation-attempt branch Oct 6, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment