Hide JanetMarshalContext implementation details#1747
Merged
Conversation
Member
|
So this looks functionally fine, but I don't want to break backwards compatibility at the API level. I think some kind of migration at the API level would be desirable to enable this though, perhaps an alias like |
This hides JanetMarshalContext behind an opaque pointer. User of this struct should use the public API instead of using those internal implementation details.
Contributor
Author
|
I see, then I think I will revert my |
ba52600 to
7e17376
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This hides JanetMarshalContext behind an opaque pointer, and splits it into two separate Context type for marshal and unmarshal.
The motivation was my confusion while working with this type because the same context type was used for both marshal/unmarshal context, despite containing almost nothing in common, and you cannot certainly use a context that was created for marshal operation for unmarshalling.
This will break current users with compile errors due to the different pointer type on the vtable now, but the ABI should remain the same with the pointer argument.
I hope this is not a very controversal change :D I noticed this when trying to stub out the implementation in my Zig port, so thought I would try to also upstream this while I can