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

Add support for custom serialisation (RFC 21) #1839

Merged
merged 1 commit into from
Jun 5, 2017
Merged

Add support for custom serialisation (RFC 21) #1839

merged 1 commit into from
Jun 5, 2017

Conversation

aturley
Copy link
Member

@aturley aturley commented Apr 14, 2017

This commit adds support for custom serialisation. Custom
serialisation lets the user provide methods that are called by Pony's
serialisation and deserialisation system in order to add additional
information that otherwise would not be serialized. The primary goal
of this system is to provide a means of serialising data stored in
Pointer member variables.

Praetonus
Praetonus reviewed Apr 14, 2017
View reviewed changes
Copy link
Member

@Praetonus Praetonus left a comment

Choose a reason for hiding this comment

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

I think the new test would be better as a compiler test instead of a standard library test. This would avoid having functions supporting tests in libponyrt. It can be implemented in the compiler test suite as a JIT test. Let me know if you need additional directions on how to implement it.

@SeanTAllen
Copy link
Member

I'm going to slap a party emoji on this.

@aturley
Copy link
Member Author

aturley commented Apr 14, 2017

God damn it C++ ... in my defense, this builds with clang++, but g++ needs an include to make it work.

@aturley aturley force-pushed the custom-serialization branch 4 times, most recently from f04aada to 867d95e Compare April 14, 2017 17:32
@aturley
Copy link
Member Author

aturley commented Apr 14, 2017

@Praetonus is there an good example of how you think I should do this?

@SeanTAllen SeanTAllen mentioned this pull request Apr 15, 2017
Closed
@Praetonus
Copy link
Member

You can look at the tests in codegen_trace.cc. The basic idea is to set the runtime's exit code (via pony_exitcode) when the test succeeds and then use the gtest API to test the exit code value to report the test result. If you need to use C functions in the Pony program, you should declare them in an extern "C" block and with EXPORT_SYMBOL in their signature.

@Praetonus Praetonus mentioned this pull request Apr 18, 2017
Closed
@SeanTAllen
Copy link
Member

Given the blocking issue #1843, I'd like to propose that this proceed forward without the compiler tests for now and just have the standard library tests.

@SeanTAllen
Copy link
Member

@aturley if this get merged before JIT tests, can you add an issue for adding JIT tests.

@chalcolith
Copy link
Member

This patch will fix the Windows build. I have tested it on Ubuntu as well.
custser.diff.txt

@Praetonus
Copy link
Member

I've opened a PR resolving the blocking issue with compiler tests.

@aturley aturley force-pushed the custom-serialization branch from 867d95e to fb2e690 Compare April 20, 2017 22:05
@aturley
Copy link
Member Author

aturley commented Apr 20, 2017

OK, now that #1843 has been resolved I've made this a compiler test, hopefully everything builds.

Praetonus
Praetonus reviewed Apr 20, 2017
View reviewed changes
test/libponyc/codegen.cc Outdated
virtual void SetUp()
{
PassTest::SetUp();
set_builtin(NULL);
Copy link
Member

Choose a reason for hiding this comment

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

I'd recommend putting this in the new test instead, before TEST_COMPILE. It would avoid having to compile the real builtin for every CodegenTest test and instead compile it only for the test that needs it, avoiding an increase in execution time for every test.

Copy link
Member Author

Choose a reason for hiding this comment

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

Seems reasonable. I have made this change.

@SeanTAllen
Copy link
Member

SeanTAllen commented Apr 25, 2017 •
edited
Loading

@aturley is a PR the tutorial and another for Pony patterns all that is left?

This should be taught as part of the C FFI documentation in the tutorial
 because it is intended to be used by objects that already interact with 
the C FFI in some way. There should also be a Pony pattern that 
addresses how to serialize and deserialize objects that have Pointer fields.

@aturley aturley force-pushed the custom-serialization branch from fb2e690 to eba92a0 Compare May 15, 2017 16:04
@aturley
Copy link
Member Author

aturley commented May 15, 2017

Some things came up and I fell behind on this ...

@SeanTAllen yes, if folks are happy with this then I can do the tutorial and patterns.

@aturley aturley force-pushed the custom-serialization branch from eba92a0 to f83ce08 Compare May 15, 2017 17:35
@aturley aturley force-pushed the custom-serialization branch from f83ce08 to 9b95f03 Compare June 2, 2017 21:23
Praetonus
Praetonus reviewed Jun 5, 2017
View reviewed changes
src/libponyc/pass/serialisers.c Outdated
return false;
}

if(ast_id(ast_serialise_space) != TK_FUN && ast_id(ast_serialise) != TK_FUN &&
Copy link
Member

Choose a reason for hiding this comment

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

The goal of this check is unclear to me. In any case the function will return true.

@aturley
Add support for custom serialisation (RFC 21)
c733761 
This commit adds support for custom serialisation. Custom
serialisation lets the user provide methods that are called by Pony's
serialisation and deserialisation system in order to add additional
information that otherwise would not be serialized. The primary goal
of this system is to provide a means of serialising data stored in
`Pointer` member variables.
@aturley aturley force-pushed the custom-serialization branch from 9b95f03 to c733761 Compare June 5, 2017 20:29
Praetonus
Praetonus approved these changes Jun 5, 2017
View reviewed changes
Copy link
Member

@Praetonus Praetonus left a comment

Choose a reason for hiding this comment

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

Looks good to me and the tests are passing, let's merge. Thank you for your work @aturley!

@Praetonus Praetonus added the changelog - added Automatically add "Added" CHANGELOG entry on merge label Jun 5, 2017
@Praetonus Praetonus merged commit 2736aed into master Jun 5, 2017
ponylang-main added a commit that referenced this pull request Jun 5, 2017
@ponylang-main
Update CHANGELOG for PR #1839 [skip ci]
f5ca8a9
@SeanTAllen SeanTAllen mentioned this pull request Jun 5, 2017
Closed
@SeanTAllen SeanTAllen deleted the custom-serialization branch June 10, 2017 23:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Praetonus Praetonus Praetonus approved these changes

Assignees
No one assigned
Labels
changelog - added Automatically add "Added" CHANGELOG entry on merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@aturley @SeanTAllen @Praetonus @chalcolith