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: Don't expose Tox_System in the public API #2741

Merged
merged 1 commit into from
Mar 17, 2024

Conversation

nurupo
Copy link
Member

@nurupo nurupo commented Mar 17, 2024

It makes no sense to include it in the public API as clients can't make any meaningful use of it via public API, it can only be used if one also includes other internal/private headers that we don't install.

It's used only in the testing code, which has access to the internal headers.

Fixes #2739, at least to some degree. I decided against moving things to a separate tox_testing.h and leaving only things in tox_private.h that we are fine with clients using, as otherwise tox_lock() / tox_unlock() would have to be moved out of tox_private.h to somewhere else, but tox_private.h actually sounds like the right place for them, naming-wise. So perhaps it's fine if we have things in tox_private.h that we don't want clients to use.


This change is Reviewable

@nurupo nurupo added this to the v0.2.19 milestone Mar 17, 2024
@nurupo nurupo force-pushed the hide-tox-system branch 2 times, most recently from 208d3e7 to 331381d Compare March 17, 2024 15:16
@nurupo nurupo changed the title Don't expose Tox_System in the public API refactor: Don't expose Tox_System in the public API Mar 17, 2024
It makes no sense to include it in the public API as clients can't make
any meaningful use of it via public API, it can only be used if one also
includes other internal/private headers that we don't install.

It's used only in the testing code, which has access to the internal
headers.

Fixes #2739, at least to some degree. I decided against moving things to
a separate `tox_testing.h` and leaving only things in `tox_private.h`
that we are fine with clients using, as otherwise `tox_lock()` /
`tox_unlock()` would have to be moved out of `tox_private.h` to
somewhere else, but `tox_private.h` actually sounds like the right place
for them, naming-wise. So perhaps it's fine if we have things in
`tox_private.h` that we don't want clients to use.
@nurupo nurupo added api break Change breaks API or ABI and removed api break Change breaks API or ABI labels Mar 17, 2024
@nurupo
Copy link
Member Author

nurupo commented Mar 17, 2024

Not sure if this should be marked as API break since we haven't made a release with Tox_System yet?

Copy link

codecov bot commented Mar 17, 2024

Codecov Report

Attention: Patch coverage is 64.00000% with 9 lines in your changes are missing coverage. Please review.

Project coverage is 73.09%. Comparing base (a3d1b85) to head (0ec4978).

Files Patch % Lines
toxcore/tox.c 43.75% 9 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2741      +/-   ##
==========================================
+ Coverage   73.04%   73.09%   +0.05%     
==========================================
  Files         149      149              
  Lines       30516    30533      +17     
==========================================
+ Hits        22290    22319      +29     
+ Misses       8226     8214      -12     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Green-Sky
Copy link
Member

It's used only in the testing code

This is not exactly true, system is supposed to be publicly/privately accessible.
Similar to other "experimental" options.

@nurupo
Copy link
Member Author

nurupo commented Mar 17, 2024

@Green-Sky I'm just stating the fact that it is currently used only in the testing code. I did a grep for "tox_options_set_operating_system" and that's the only code that used that function. (If you search for Tox_System, it's also being used in events).

Also, see #2739 (comment) - there is no way for any external code to use Tox_System, as struct Random, struct Network and struct Memory are not defined in any public API. Clients can't create a new instance of Tox_System, they can only get the system default / Tox's, and even then there is nothing they can do with it as it's opaque without those struct definitions, besides perhaps passing it back to Tox.

It would indeed be nice for clients to be able to provide their own Tox_System implementations, but with these structs being private, it doesn't seem like we are there yet. Once we get there, once clients are able to do something useful with it, we can add it back to Tox_Options.

@nurupo nurupo marked this pull request as ready for review March 17, 2024 16:44
@Green-Sky
Copy link
Member

Green-Sky commented Mar 17, 2024

there is no way for any external code to use Tox_System, as struct Random, struct Network and struct Memory are not defined in any public API

Yea that's bad, I had to include mem.h to be able to provide a custom allocator, just yesterday.

It would indeed be nice for clients to be able to provide their own Tox_System implementations, but with these structs being private, it doesn't seem like we are there yet. Once we get there, once clients are able to do something useful with it, we can add it back to Tox_Options.

Fine.

Copy link
Member

@Green-Sky Green-Sky left a comment

Choose a reason for hiding this comment

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

sprint to release

@nurupo nurupo merged commit 0ec4978 into TokTok:master Mar 17, 2024
62 of 63 checks passed
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.

Tox_Options.operating_system is not clear about it being an experimental option
3 participants