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

Fix (de)serialization of aggregate types with functions returning by ref #817

Merged
merged 4 commits into from Sep 11, 2014

Conversation

Projects
None yet
3 participants
@ReneZwanenburg
Contributor

ReneZwanenburg commented Sep 10, 2014

This fixes a number of corner cases present in the serialization module, original report is issue #802

ReneZwanenburg added some commits Sep 10, 2014

Fix some corner cases when (de)serializing aggregate types
The serializer now ignores member functions which are templates, return
by ref, or don't have the @Property attribute.
@ReneZwanenburg

This comment has been minimized.

Show comment
Hide comment
@ReneZwanenburg

ReneZwanenburg Sep 10, 2014

Contributor

I just came across #813. This PR probably makes it redundant.

Contributor

ReneZwanenburg commented Sep 10, 2014

I just came across #813. This PR probably makes it redundant.

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Sep 10, 2014

Member

Thanks for doing the fix! What definitely is needed is a set of test cases with the problematic cases, because it is quite likely that this part will have to be changed again in the future. So if you could add any of those that you have stumbled over, that would be great.

Member

s-ludwig commented Sep 10, 2014

Thanks for doing the fix! What definitely is needed is a set of test cases with the problematic cases, because it is quite likely that this part will have to be changed again in the future. So if you could add any of those that you have stumbled over, that would be great.

@ReneZwanenburg

This comment has been minimized.

Show comment
Hide comment
@ReneZwanenburg

ReneZwanenburg Sep 10, 2014

Contributor

Sure. I really should get in the habit of writing those immediately :)

The unittests use the Json serializer instead of the TestSerializer. When using the TestSerializer to deserialize anything, even a POD struct, I'm getting an ICE. That may or may not happen with 2.066, I'm still using 2.055 because my main project is hitting a regression.

Contributor

ReneZwanenburg commented Sep 10, 2014

Sure. I really should get in the habit of writing those immediately :)

The unittests use the Json serializer instead of the TestSerializer. When using the TestSerializer to deserialize anything, even a POD struct, I'm getting an ICE. That may or may not happen with 2.066, I'm still using 2.055 because my main project is hitting a regression.

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Sep 11, 2014

Member

Okay, thanks. I'll try to complete the test serializer later and then switch the unit tests (deserialization is only partially implemented right now). Hopefully the Dustmite run of @ColdenCullen for #813 will yield an additional test case.

Member

s-ludwig commented Sep 11, 2014

Okay, thanks. I'll try to complete the test serializer later and then switch the unit tests (deserialization is only partially implemented right now). Hopefully the Dustmite run of @ColdenCullen for #813 will yield an additional test case.

s-ludwig added a commit that referenced this pull request Sep 11, 2014

Merge pull request #817 from ReneZwanenburg/serialization-template-re…
…f-fix

Fix (de)serialization of aggregate types with functions returning by ref

@s-ludwig s-ludwig merged commit 973fdbd into vibe-d:master Sep 11, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details
@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Sep 11, 2014

Member

I'm currently reorganizing this again, trying to make the rules a bit more general. Regarding the rejection of parameter-less templates, did you add that because you think that those shouldn't be treated as fields, or because they resulted in compilation errors? I'm asking because I'm not sure if they really shouldn't be treated as fields.

Member

s-ludwig commented Sep 11, 2014

I'm currently reorganizing this again, trying to make the rules a bit more general. Regarding the rejection of parameter-less templates, did you add that because you think that those shouldn't be treated as fields, or because they resulted in compilation errors? I'm asking because I'm not sure if they really shouldn't be treated as fields.

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Sep 11, 2014

Member

Another one is @property ref T prop() which I would consider a valid RW field. Any reason why that should instead be rejected?

Member

s-ludwig commented Sep 11, 2014

Another one is @property ref T prop() which I would consider a valid RW field. Any reason why that should instead be rejected?

@ReneZwanenburg

This comment has been minimized.

Show comment
Hide comment
@ReneZwanenburg

ReneZwanenburg Sep 11, 2014

Contributor

Regarding parameter-less templates, that check was supposed to filter out variadic templates which can't be treated as fields. I didn't take parameter-less templates into account.. I'm not sure if those should work, but I don't know of a way to detect if something is a variadic template.

@property ref should work too. I just checked, when a property returns by ref it does not result in the Error: functions cannot return a function error I was seeing earlier. So the check for returning by ref can safely be removed altogether I think.

Contributor

ReneZwanenburg commented Sep 11, 2014

Regarding parameter-less templates, that check was supposed to filter out variadic templates which can't be treated as fields. I didn't take parameter-less templates into account.. I'm not sure if those should work, but I don't know of a way to detect if something is a variadic template.

@property ref should work too. I just checked, when a property returns by ref it does not result in the Error: functions cannot return a function error I was seeing earlier. So the check for returning by ref can safely be removed altogether I think.

@ReneZwanenburg ReneZwanenburg deleted the ReneZwanenburg:serialization-template-ref-fix branch Sep 11, 2014

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Sep 11, 2014

Member

OK. Turns out that parameter-less template properties are rejected by DMD anyway as "is not a property".

Member

s-ludwig commented Sep 11, 2014

OK. Turns out that parameter-less template properties are rejected by DMD anyway as "is not a property".

@ColdenCullen

This comment has been minimized.

Show comment
Hide comment
@ColdenCullen

ColdenCullen Sep 11, 2014

Contributor

This does seem like it will potentially fix my problem. I'll try pulling from master to see if that fixes it. Fingers crossed.

So far dustmite has been running on a top-of-the-line machine for about 48 hours with no sign of stopping, so I think I'm just going to kill it.

Contributor

ColdenCullen commented Sep 11, 2014

This does seem like it will potentially fix my problem. I'll try pulling from master to see if that fixes it. Fingers crossed.

So far dustmite has been running on a top-of-the-line machine for about 48 hours with no sign of stopping, so I think I'm just going to kill it.

@ColdenCullen

This comment has been minimized.

Show comment
Hide comment
@ColdenCullen

ColdenCullen Sep 11, 2014

Contributor

Yes, this fixed it, thanks! Can we get a beta.3 release with this in it?

Contributor

ColdenCullen commented Sep 11, 2014

Yes, this fixed it, thanks! Can we get a beta.3 release with this in it?

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Sep 11, 2014

Member

I'll just still wait for #818 and then tag a new version.

And thanks for verifying, of course. With the new unit tests this should be much more robust in the future. Unfortunately things like these pretty much depend exclusively on trial and error, so they are also prone to break with new compiler releases, because they often depend on unspecified behavior.

Member

s-ludwig commented Sep 11, 2014

I'll just still wait for #818 and then tag a new version.

And thanks for verifying, of course. With the new unit tests this should be much more robust in the future. Unfortunately things like these pretty much depend exclusively on trial and error, so they are also prone to break with new compiler releases, because they often depend on unspecified behavior.

@ColdenCullen

This comment has been minimized.

Show comment
Hide comment
@ColdenCullen

ColdenCullen Sep 11, 2014

Contributor

No worries, I'm always happy to help. I'm in no rush, I just don't want to be using a ~master version spec forever.

Contributor

ColdenCullen commented Sep 11, 2014

No worries, I'm always happy to help. I'm in no rush, I just don't want to be using a ~master version spec forever.

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