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

refactor: remove tox_options_default(...) and make Tox_Options opaque #1825

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

sudden6
Copy link

@sudden6 sudden6 commented Dec 29, 2021

tox_options_new(...) already returns a default initialized options
struct and this function makes it impossible to malloc buffers in
Tox_Options, so remove it.

Required for #909

Fixes #273


This change is Reviewable

@auto-add-label auto-add-label bot added the refactor Refactoring production code, eg. renaming a variable, not affecting semantics label Dec 29, 2021
@sudden6 sudden6 added the api break Change breaks API or ABI label Dec 29, 2021
tox_options_new(...) already returns a default initialized options
struct and this function makes it impossible to malloc buffers in
Tox_Options, so remove it.
@sudden6 sudden6 force-pushed the remove_options_default branch from 9372c8d to 6d9bd44 Compare December 29, 2021 23:55
@nurupo
Copy link
Member

nurupo commented Dec 30, 2021

You should not malloc buffers in Tox_Options. See #909 (comment).

@nurupo nurupo closed this Dec 30, 2021
@sudden6
Copy link
Author

sudden6 commented Dec 30, 2021

Reopening, resoning in: #909 (comment)

Edit: @nurupo even if you leave out #909 removing this function still makes sense, since it provides no real value (reseting to defaults is done in tox_options_new(...)) and prevents further evolution of the API, because of the uninitialized pointers problem.

@sudden6 sudden6 reopened this Dec 30, 2021
@iphydf iphydf added this to the v0.3.0 milestone Dec 30, 2021
@sudden6 sudden6 changed the title refactor: remove tox_options_default(...) refactor: remove tox_options_default(...) and make Tox_Options opaque Dec 30, 2021
@sudden6 sudden6 force-pushed the remove_options_default branch 2 times, most recently from 5911606 to 81fe998 Compare December 30, 2021 15:29
@sudden6
Copy link
Author

sudden6 commented Dec 30, 2021

Updated this PR with the logical next step of making Tox_Options opaque.

Copy link
Member

@JFreegman JFreegman left a comment

Choose a reason for hiding this comment

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

Reviewed 19 of 19 files at r2, all commit messages.
Reviewable status: 0 of 1 approvals obtained (waiting on @sudden6)


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

/**
 * This struct contains all the startup options for Tox. You must tox_options_new to
 * allocate an object of this type and tox_options_free to free it.

"you must use"

and "and use"

@nurupo
Copy link
Member

nurupo commented Dec 30, 2021

Edit: @nurupo even if you leave out #909 removing this function still makes sense, since it provides no real value (reseting to defaults is done in tox_options_new(...)) and prevents further evolution of the API, because of the uninitialized pointers problem.

Ok, I'm leaving #909 aside for now then. What is the uninitialized pointers problem? Are you referring back to #909 even though you said "if you leave out #909", or is there some other problem I don't know?

Anyway, we do want to make Tox_Options opaque, so that's a nice change to have. tox_options_default() becomes a bit pointless indeed after that change. I guess it could be useful to reset the object to its default values without having to free/new it, but it's not obvious to me why a user would want to do that, doesn't really add much value, so yeah, let's remove it.

@sudden6 sudden6 force-pushed the remove_options_default branch from 81fe998 to 547330e Compare December 30, 2021 21:13
@sudden6
Copy link
Author

sudden6 commented Dec 30, 2021

What is the uninitialized pointers problem? Are you referring back to #909 even though you said "if you leave out #909", or is there some other problem I don't know?

My mistake, after making Tox_Options opaque, all pointers are either NULL or valid since we initialize them in tox_options_new(...).

I'd still remove tox_options_default(...) to keep the API as minimal as possible.

@JFreegman thx, fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api break Change breaks API or ABI refactor Refactoring production code, eg. renaming a variable, not affecting semantics
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make Tox_Options struct opaque: remove definition from tox.h, leaving only declaration
4 participants