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

Use tox_options_set_* instead of direct member access. #333

Merged
merged 3 commits into from
Dec 23, 2016

Conversation

iphydf
Copy link
Member

@iphydf iphydf commented Dec 15, 2016

Also added a tox_options_copy function for cloning an options object.
This can be useful when creating several Tox instances with slightly
varying options.


This change is Reviewable

@iphydf iphydf added this to the v0.1.2 milestone Dec 15, 2016
@iphydf iphydf force-pushed the opts-setters branch 8 times, most recently from 505b7ad to 1b8159d Compare December 17, 2016 17:34
@robinlinden
Copy link
Member

:lgtm_strong:


Reviewed 12 of 12 files at r1.
Review status: 6 of 12 files reviewed at latest revision, all discussions resolved, some commit checks failed.


Comments from Reviewable

@nurupo
Copy link
Member

nurupo commented Dec 22, 2016

Reviewed 6 of 12 files at r1, 6 of 6 files at r2.
Review status: all files reviewed at latest revision, 1 unresolved discussion.


toxcore/tox.h, line 730 at r2 (raw file):

/**
 * Allocates a new Tox_Options object and initialises it with the values from the

Doesn't allocate anything.


Comments from Reviewable

@nurupo
Copy link
Member

nurupo commented Dec 22, 2016

Also added a tox_options_copy function for cloning an options object.
This can be useful when creating several Tox instances with slightly
varying options.

But you can create Tox_Options struct, set it the way you need, create a Tox instance with it, then slightly modify this Tox_Options and create another Tox instance with it, without need for cloning.


Comments from Reviewable

@nurupo
Copy link
Member

nurupo commented Dec 22, 2016

Also, what is the point of tox_api.c? Why it's separate from tox.c? It makes sense to move the entire Tox_Options in a separate file(s), tox_options.[h|c] perhaps, but this is not just Tox_Options in there but also constants that are totally unrelated to Tox_Options, which makes me question what is the purpose of tox_api.c and how it's different from the purpose of tox.c.


Review status: all files reviewed at latest revision, 1 unresolved discussion.


Comments from Reviewable

Also added a `tox_options_copy` function for cloning an options object.
This can be useful when creating several Tox instances with slightly
varying options.
@iphydf iphydf force-pushed the opts-setters branch 2 times, most recently from acce1a6 to 25cadff Compare December 22, 2016 16:25
@iphydf
Copy link
Member Author

iphydf commented Dec 22, 2016

The point of tox_api.c (and we can change the name if you have a better suggestion) is that it contains all the code that could be autogenerated by apidsl. Perhaps some of the code (like new) can't or shouldn't actually be autogenerated, but all the functions for named constants could and should be.


Review status: 6 of 10 files reviewed at latest revision, 1 unresolved discussion.


Comments from Reviewable

@iphydf iphydf force-pushed the opts-setters branch 2 times, most recently from dd0f562 to c334c78 Compare December 22, 2016 20:21
@nurupo
Copy link
Member

nurupo commented Dec 23, 2016

:lgtm_strong:


Reviewed 5 of 6 files at r3.
Review status: all files reviewed at latest revision, 2 unresolved discussions.


auto_tests/helpers.h, line 59 at r3 (raw file):

    if (options != NULL) {
        // TODO(iphydf): don't dereference Tox_Options pointers, as the type
        // will become opaque soon.

Why don't we want to avoid modifying the Tox_Options struct in this helper function for tests? We could just make it the default behavior of this function and document it, don't really see any harm in it.


Comments from Reviewable

@nurupo
Copy link
Member

nurupo commented Dec 23, 2016

auto_tests/helpers.h, line 59 at r3 (raw file):

Previously, nurupo wrote…

Why don't we want to avoid modifying the Tox_Options struct in this helper function for tests? We could just make it the default behavior of this function and document it, don't really see any harm in it.

*Why do we want to avoid


Comments from Reviewable

@nurupo
Copy link
Member

nurupo commented Dec 23, 2016

:lgtm_strong:


Review status: 9 of 10 files reviewed at latest revision, 2 unresolved discussions, all commit checks successful.


Comments from Reviewable

@nurupo
Copy link
Member

nurupo commented Dec 23, 2016

Reviewed 1 of 1 files at r4.
Review status: all files reviewed at latest revision, 2 unresolved discussions, all commit checks successful.


Comments from Reviewable

@iphydf iphydf merged commit ce49e88 into TokTok:master Dec 23, 2016
@iphydf iphydf deleted the opts-setters branch December 23, 2016 02:30
This pull request was closed.
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.

3 participants