Skip to content

refactor: Don't expose Tox_System in the public API#2741

Merged
nurupo merged 1 commit into
TokTok:masterfrom
nurupo:hide-tox-system
Mar 17, 2024
Merged

refactor: Don't expose Tox_System in the public API#2741
nurupo merged 1 commit into
TokTok:masterfrom
nurupo:hide-tox-system

Conversation

@nurupo

@nurupo nurupo commented Mar 17, 2024

Copy link
Copy Markdown
Member

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

nurupo commented Mar 17, 2024

Copy link
Copy Markdown
Member Author

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

@codecov

codecov Bot commented Mar 17, 2024

Copy link
Copy Markdown

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
Copy Markdown
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

nurupo commented Mar 17, 2024

Copy link
Copy Markdown
Member Author

@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

Green-Sky commented Mar 17, 2024

Copy link
Copy Markdown
Member

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.

@Green-Sky Green-Sky left a comment

Copy link
Copy Markdown
Member

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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