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.

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

Move load_state and its helper functions to their own module. #993

Merged
merged 1 commit into from
Jul 9, 2018

Conversation

iphydf
Copy link
Member

@iphydf iphydf commented Jul 9, 2018

This change is Reviewable

@iphydf iphydf added this to the v0.2.x milestone Jul 9, 2018
@iphydf iphydf force-pushed the state-module branch 7 times, most recently from ed4d4df to 5bb4554 Compare July 9, 2018 18:17
Copy link

@sudden6 sudden6 left a comment

Choose a reason for hiding this comment

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

Reviewed 8 of 10 files at r1.
Reviewable status: 0 of 1 LGTMs obtained


toxcore/state.h, line 9 at r1 (raw file):

extern "C" {
#endif

Can you add a comment what the purpose of this module is?


toxcore/state.c, line 63 at r1 (raw file):

}

uint16_t lendian_to_host16(uint16_t lendian)

why re-implement functionality we already have in net_pack_*? I'd prefer to do this architecture dependent serialization at exactly one place.

Copy link
Member Author

@iphydf iphydf left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 1 LGTMs obtained


toxcore/state.c, line 63 at r1 (raw file):

Previously, sudden6 wrote…

why re-implement functionality we already have in net_pack_*? I'd prefer to do this architecture dependent serialization at exactly one place.

Note that I moved these functions from util.c, I didn't reimplement anything here. Also note that these functions convert to little endian, since the save data format has a mix of big and little endian (and maybe some machine dependent ones that are de-facto little endian.

I don't want these in network.h because they have nothing to do with sending things over the network, but everything to do with writing state to a byte array.

Also note that these functions are not inherently architecture dependent. Their current implementation is, as an optimisation, because clearly toxcore was designed to write gigabytes of save data per second :) (more seriously: I'm going to rewrite these in a portable way).

@sudden6
Copy link

sudden6 commented Jul 9, 2018


toxcore/state.c, line 63 at r1 (raw file):

Previously, iphydf wrote…

Note that I moved these functions from util.c, I didn't reimplement anything here. Also note that these functions convert to little endian, since the save data format has a mix of big and little endian (and maybe some machine dependent ones that are de-facto little endian.

I don't want these in network.h because they have nothing to do with sending things over the network, but everything to do with writing state to a byte array.

Also note that these functions are not inherently architecture dependent. Their current implementation is, as an optimisation, because clearly toxcore was designed to write gigabytes of save data per second :) (more seriously: I'm going to rewrite these in a portable way).

Maybe we should then create a pack module that does exactly this byte packing for saving and network? Maybe not now. IMO we should at least leave a comment to unify this.

Copy link
Member Author

@iphydf iphydf left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 1 LGTMs obtained


toxcore/state.h, line 9 at r1 (raw file):

Previously, sudden6 wrote…

Can you add a comment what the purpose of this module is?

Done.


toxcore/state.c, line 63 at r1 (raw file):

Previously, sudden6 wrote…

Maybe we should then create a pack module that does exactly this byte packing for saving and network? Maybe not now. IMO we should at least leave a comment to unify this.

I think it's good to have state_pack_* functions here (renaming the lendian functions here) and net_pack_* in the network module.

Copy link

@sudden6 sudden6 left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 10 files at r1, 1 of 1 files at r2.
Reviewable status: 0 of 1 LGTMs obtained


toxcore/state.c, line 63 at r1 (raw file):

Previously, iphydf wrote…

I think it's good to have state_pack_* functions here (renaming the lendian functions here) and net_pack_* in the network module.

ok, I can live with that for now.

@iphydf iphydf merged commit c8697cc into TokTok:master Jul 9, 2018
@iphydf iphydf deleted the state-module branch July 9, 2018 21:01
@iphydf iphydf modified the milestones: v0.2.x, v0.2.4 Jul 16, 2018
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.

None yet

2 participants