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

Make Tox_Options struct opaque: remove definition from tox.h, leaving only declaration #273

Open
nurupo opened this issue Nov 14, 2016 · 8 comments · May be fixed by #1825
Open

Make Tox_Options struct opaque: remove definition from tox.h, leaving only declaration #273

nurupo opened this issue Nov 14, 2016 · 8 comments · May be fixed by #1825
Assignees
Labels
cleanup Internal code cleanup, possibly affecting semantics, e.g. deleting a deprecated feature. P2 Medium priority suggestion Suggestions
Milestone

Comments

@nurupo
Copy link
Member

nurupo commented Nov 14, 2016

There is no reason for Tox_Options not to be an opaque pointer. Accessing the struct's members directly breaks ABI compatibility and adding members to the struct in the middle of it, as it's done in #226 doesn't, help that cause much.

@nurupo nurupo added this to the v0.1 milestone Nov 14, 2016
@GrayHatter
Copy link

Agreed, but I don't think v0.1 is necessary, this could be merged in much earlier than that. This is an easy beneficial change.

Alternatively, we could drop the idea of making clients manage the options at all, and either A) rewrite tox_new() to generate a non-running instance with the default options, allowing the client to set options before the first tox_iterate() and have the first call of tox_iterate() actually do the initiation currently done by tox_new(). Or B) same as the above but add tox_init() so the flow would be tox_new() -> set_opts() -> tox_init() -> tox_iterate(). Then clients don't even need to know about that struct.

@nurupo
Copy link
Member Author

nurupo commented Nov 14, 2016

A) Is bad because nothing prevents users from trying to set options after tox_iterate() is called. The option setting could be simply ignored at that point, but imposing a restriction that option function should be called only before tox_iterate() sounds like artificial limiting.

B) Pretty much the same issue as in A).

I like the idea of the Tox_Options struct, it can be used only in tox_new() call, so there is no artificial restrictions like "Don't set options when iterated at least once!", instead this restriction is enforced by the way the code is written, you simply can't set options at any other point, nothing else accepts Tox_Options, so I don't think we need to remove Tox_Options.

@GrayHatter GrayHatter modified the milestones: v0.0.5, v0.1 Nov 14, 2016
@GrayHatter GrayHatter added suggestion Suggestions P1 High priority labels Nov 14, 2016
@GrayHatter GrayHatter changed the title Remove Tox_Options struct definition from the tox.h, leaving only declatation Make Tox_Options struct opaque -- Remove Tox_Options struct definition from the tox.h, leaving only declatation Nov 14, 2016
@GrayHatter
Copy link

GrayHatter commented Nov 14, 2016

@nurupo I renamed this to something more obvious, and moved the milestone to v0.0.5. If we're going to do this. We might as well get it over with now.

Edit -- uTox already supports this method.

@iphydf iphydf modified the milestones: v0.1, v0.0.5 Nov 15, 2016
@iphydf
Copy link
Member

iphydf commented Nov 15, 2016

For v0.0.5:

  • Mark the struct as deprecated.
  • Find a way to avoid duplication of the struct contents (make apidsl generate .c files?).

For v0.1, we can then actually remove it. Its existence doesn't do much harm at the moment, and delaying its removal gives client and bindings authors time to update.

CC: @zetok @SkyzohKey for client updates.

@iphydf
Copy link
Member

iphydf commented Nov 15, 2016

AFAICS, this will need:

  • marking the struct as deprecated;
  • additions to apidsl (e.g. needs to generate a .c file with the struct definition, malloc/free functions, some way to hook in the "default" function for the allocation function);
  • changes to the build system (needs to be aware that a .c file will be generated);
  • tox.c to use the accessor functions instead of accessing pointer members directly;
  • the clients to update to use the accessor functions;
  • all bindings to update as well.

@iphydf iphydf self-assigned this Nov 15, 2016
@SkyzohKey
Copy link

SkyzohKey commented Nov 19, 2016

I'd update Ricin once I know what changes needs to be made. :)

@iphydf iphydf modified the milestones: v0.1.0, v0.2.0 Dec 10, 2016
@iphydf iphydf changed the title Make Tox_Options struct opaque -- Remove Tox_Options struct definition from the tox.h, leaving only declatation Make Tox_Options struct opaque: remove definition from tox.h, leaving only declaration Dec 18, 2016
@iphydf
Copy link
Member

iphydf commented Dec 18, 2016

#333 does point 4 (tox.c using accessor functions).

@iphydf iphydf modified the milestone: v0.2.0 Jan 10, 2017
@iphydf iphydf added this to the v0.2.x milestone Jul 16, 2018
@iphydf iphydf added refactor Refactoring production code, eg. renaming a variable, not affecting semantics cleanup Internal code cleanup, possibly affecting semantics, e.g. deleting a deprecated feature. and removed code cleanup refactor Refactoring production code, eg. renaming a variable, not affecting semantics labels May 4, 2020
@iphydf iphydf assigned sudden6 and unassigned iphydf Feb 4, 2022
@iphydf iphydf added P3 Low priority and removed P1 High priority labels Feb 4, 2022
@iphydf
Copy link
Member

iphydf commented Feb 4, 2022

#1825 does this. Assigned to @sudden6 because that PR fixes this issue.

@iphydf iphydf added P2 Medium priority and removed P3 Low priority labels Feb 4, 2022
@iphydf iphydf modified the milestones: v0.2.x, v0.3.0 Feb 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cleanup Internal code cleanup, possibly affecting semantics, e.g. deleting a deprecated feature. P2 Medium priority suggestion Suggestions
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants