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

[nurupo]

There is an experimental section at the bottom of Tox_Options:

c-toxcore/toxcore/tox.h

Lines 662 to 692 in ad4921d

	/**
	* These options are experimental, so avoid writing code that depends on
	* them. Options marked "experimental" may change their behaviour or go away
	* entirely in the future, or may be renamed to something non-experimental
	* if they become part of the supported API.
	*/
	/**
	* Make public API functions thread-safe using a per-instance lock.
	*
	* Default: false.
	*/
	bool experimental_thread_safety;
	/**
	* Low level operating system functionality such as send/recv, random
	* number generation, and memory allocation.
	*/
	const Tox_System *operating_system;
	/**
	* Enable saving DHT-based group chats to Tox save data (via `tox_get_savedata`).
	* This format will change in the future, so don't rely on it.
	*
	* As an alternative, clients can save the group chat ID in client-owned
	* savedata. Then, when the client starts, it can use `tox_group_join`
	* with the saved chat ID to recreate the group chat.
	*
	* Default: false.
	*/
	bool experimental_groups_persistence;
	};

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 #2735 (comment).

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]

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

[nurupo]

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:

c-toxcore/toxcore/tox.h

Lines 508 to 515 in ad4921d

	/**
	* @brief Operating system functions used by Tox.
	*
	* This struct is opaque and generally shouldn't be used in clients, but in
	* combination with tox_private.h, it allows tests to inject non-IO (hermetic)
	* versions of low level network, RNG, and time keeping functions.
	*/
	typedef struct Tox_System Tox_System;

c-toxcore/toxcore/tox.h

Lines 762 to 764 in ad4921d

	const Tox_System *tox_options_get_operating_system(const Tox_Options *options);
	void tox_options_set_operating_system(Tox_Options *options, const Tox_System *operating_system);

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

c-toxcore/toxcore/tox_private.h

Lines 19 to 34 in ad4921d

	typedef uint64_t tox_mono_time_cb(void *user_data);
	struct Tox_System {
	tox_mono_time_cb *mono_time_callback;
	void *mono_time_user_data;
	const struct Random *rng;
	const struct Network *ns;
	const struct Memory *mem;
	};
	Tox_System tox_default_system(void);
	void tox_lock(const Tox *tox);
	void tox_unlock(const Tox *tox);
	const Tox_System *tox_get_system(Tox *tox);

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.