TokTok / c-toxcore

The future of online communications.
https://tox.chat
GNU General Public License v3.0
2.21k stars 280 forks source link

Tox_Options.operating_system is not clear about it being an experimental option #2739

Closed nurupo closed 3 months ago

nurupo commented 3 months ago

There is an experimental section at the bottom of Tox_Options:

https://github.com/TokTok/c-toxcore/blob/ad4921dbaa2d8c98d9c3b89a113da244064f4d11/toxcore/tox.h#L662-L692

It starts with a comment that says:

  1. The following options are experimental
  2. Options marked "experimental" may change behavior or get removed
  3. Options marked "experimental" may be renamed to something non-experimental if they become part of the supported API

This sounds like all the options below that comment must be marked as "experimental" since they are all experimental.

Below we see experimental_groups_persistence and experimental_thread_safety, so by "marked experimental" I assume it means having the experimental_ prefix.

However, the operating_systems option is not prefixed with experimental_. Its naming goes against what the comment says -- that all options below the comment are experimental and thus are marked as experimental.

Due to (3), one might think that perhaps it was experimental at some point but has became a part of the supported API since it's not marked as experimental and someone just forgot to move it above the experimental section of Tox_Options. But apparently it is in fact an experimental option https://github.com/TokTok/c-toxcore/pull/2735#issuecomment-1987427061.

So I'm left very confused as what is going on with the experimental section. It's not coherent and contradicts itself. Something needs to be done to clear this up before the .19 release as this might be API breaking change depending on how it's handled, e.g. if we end up renaming operating_systems to experimental_operating_systems, along with the get/set functions.

iphydf commented 3 months ago

I think we can rename it. Nothing external uses it, it's only used in events (and maybe somewhere else inside toxcore).

nurupo commented 3 months ago

Huh, this is weirder than I thought. Now that I look into it, there is no way for clients to use const Tox_System *operating_system, even when building with -DEXPERIMENTAL_API=ON, i.e. with tox_private.h included.

Tox_System * is an opaque pointer in tox.h and there are no functions defined on it, there are only a getter and setter of it for Tox_Options:

https://github.com/TokTok/c-toxcore/blob/ad4921dbaa2d8c98d9c3b89a113da244064f4d11/toxcore/tox.h#L508-L515

https://github.com/TokTok/c-toxcore/blob/ad4921dbaa2d8c98d9c3b89a113da244064f4d11/toxcore/tox.h#L762-L764

Similar thing is happening in tox_private.h, while the struct is now defined, it consists mainly of opaque pointers:

https://github.com/TokTok/c-toxcore/blob/ad4921dbaa2d8c98d9c3b89a113da244064f4d11/toxcore/tox_private.h#L19-L34

A client at most has access to the following headers (taken from the docker-windows-mingw CI):

    |-- [4.0K]  include
    |   `-- [4.0K]  tox
    |       |-- [177K]  tox.h
    |       |-- [ 10K]  tox_dispatch.h
    |       |-- [ 26K]  tox_events.h
    |       |-- [6.4K]  tox_private.h
    |       |-- [ 26K]  toxav.h
    |       `-- [ 12K]  toxencryptsave.h

A client has no definitions for struct Random, struct Network and struct Memory, so with the little definitions that tox_private.h gives, clients are able to at most modify tox_mono_time_cb *mono_time_callback and void *mono_time_user_data, which is not very useful.

(Please correct me if I'm wrong and we actually assume that if a client has access to tox_private.h then they also have access to all of toxcore/*.h headers to get definitions of all the opaque structs, in which case I should modify the docker-windows-mingw CI to copy all the toxcore/*.h headers into include/.)

It looks like Tox_Options.operating_system is primarily used by tests, which have definitions for struct Random, struct Network and struct Memory as they include toxcore/*.h headers and thus can replace them with custom implementations.

If there is no way for a client to use Tox_Options.operating_system and its primarily use is for testing, it begs the question why Tox_Options.operating_system and Tox_System * are in tox.h at all. This seems like a wrong place for something like this. They belong in tox_private.h, or perhaps even a new non-installable tox_testing.h, if things in tox_private.h are supposed to be usable without inclusion of any of other private toxcore headers like network.h, mem.h and such. This is very sloppy.