Add region mode (for inline TUIs) (attempt 2)

[adsr]

Same approach as #114 except a new variadic function tb_init_ex is introduced which accepts a variable number of initialization options, including region mode. I think this is an improvement as it leaves room for future init options and doesn't require awkwardly calling tb_region before init.

[txgk]

Hi, Adam

I like the variadic function approach with tb_init_ex() - it will definitely make termbox2 API less loaded with configuration calls like tb_set_uattr_func(), tb_set_log_function(), etc.

While it makes the API more concise, there's also a caveat that comes with it: variadic functions require very cautious handling. For example, when we expect an int argument under the hood, the user of tb_init_ex() must always pass specifically an int (or something that promotes to it), otherwise it's undefined behavior. I'd leave a warning in the comments that warns users against passing arguments of invalid types.

The TB_INIT_TTY option expects a char * string and doesn't make it obvious who owns its memory. In the current state of things, we expect the user to manage memory for this string on their side for the whole lifetime of the termbox operation, from tb_init_ex() to tb_shutdown(). In my humble opinion, it'd be better if we copied the string from the user during tb_init_ex() and free the user from the obligation to manage memory for config strings of termbox - it really feels like the library's responsibility. Usually they document strings like that as const char * instead of char * which emphasizes that we don't depend on the pointer passed to us from the user after the call and make an internal copy instead.

Best regards, Grigory

[adsr]

Instead of a variadic, what do you think about typing all tb_init_ex options as uintptr_t? That way we could accept two params int num_opts, uintptr_t *opts. uintptr_t is safe to cast to a pointer, and should be large enough to accommodate file descriptors and any reasonable height values. Obviously not without downsides. Requires casting on both sides, might be truncation or UB in edge cases, not as terse in C, and still hands the caller a footgun if they aren't careful. However I feel like it may be 1 peg less of a footgun than the variadic version. I'll separate out this tb_init_ex part into its own PR.

Agreed on copying-ing ttypath. It's only used in the init call itself, but it's easy and cheap enough to make a copy to reduce confusion.

Thanks for the feedback.

[txgk]

Instead of a variadic, what do you think about typing all tb_init_ex options as uintptr_t? That way we could accept two params int num_opts, uintptr_t *opts. uintptr_t is safe to cast to a pointer, and should be large enough to accommodate file descriptors and any reasonable height values. Obviously not without downsides. Requires casting on both sides, might be truncation or UB in edge cases, not as terse in C, and still hands the caller a footgun if they aren't careful. However I feel like it may be 1 peg less of a footgun than the variadic version. I'll separate out this tb_init_ex part into its own PR.

You mean something like this?

#include "termbox2.h"

#define LENGTH(X) (sizeof(X) / sizeof(*X))

int tb_init_ex(size_t argc, const uintptr_t *argv);

int main(void) {

	const uintptr_t args[] = {
		TB_INIT_TTY,    (uintptr_t)"/dev/tty",
		TB_INIT_TTYFD,  (uintptr_t)13,
		TB_INIT_RFD,    (uintptr_t)0,
		TB_INIT_WFD,    (uintptr_t)1,
		TB_INIT_REGION, (uintptr_t)10,
	};

	tb_init_ex(LENGTH(args), args);

	return 0;

}

Yeah, it's definitely safer, but you're still forced to cast things explicitly sometimes, just as with the variadic approach. The good thing is that the compiler now forces you to do those casts properly, since it errors out with messages like this, for example, if you pass a char * in a uintptr_t context:

error: initialization of ‘long unsigned int’ from ‘char *’ makes integer from pointer without a cast [-Wint-conversion]

For convenience's sake, we can introduce a small macro to make the tb_init_ex invocation expose less library internals on user:

#include "termbox2.h"
typedef uintptr_t tb_arg_t;
#define tb_arg(X) ((tb_arg_t)X)

#define LENGTH(X) (sizeof(X) / sizeof(*X))

int tb_init_ex(size_t argc, const tb_arg_t *argv);

int main(void) {

	const tb_arg_t args[] = {
		TB_INIT_TTY,    tb_arg("/dev/tty"),
		TB_INIT_TTYFD,  tb_arg(13),
		TB_INIT_RFD,    tb_arg(0),
		TB_INIT_WFD,    tb_arg(1),
		TB_INIT_REGION, tb_arg(10),
	};

	tb_init_ex(LENGTH(args), args);

	return 0;

}

As you can see, with typedef uintptr_t tb_arg_t and #define tb_arg(X) ((tb_arg_t)(X)), users no longer have to think about how arguments are interpreted under the hood. To me, this looks more reliable: we can now decide whether to use uintptr_t, uintmax_t or ptrdiff_t for tb_arg_t without requiring users to change their code.

Best regards, Grigory

[stianhoiland]

I'm scoping out termbox2 as a dependency for a nascent project of mine, and I'm participating on some issues that are relevant to myself.

Why are you not using an options struct? That seems like the obvious way to simplify and centralize. Sorry if this is a dumb question in context I'm not privy to.

[adsr]

Hi @stianhoiland, not a dumb question. A struct is a good option. The one downside is ABI compat issues particularly when using termbox as a shared library. The alternatives all come with some safety trade-off. termbox already doesn't optimize for safety in all cases (e.g., there are ways to shoot yourself with tb_get_cell), so IMO one of the alternatives might fit with the existing style.

Here's a summary of the ideas so far:

// struct (ABI compat issues)
struct tb_init_opts o = {0};
o.file = "/dev/tty";
o.region_h = 3;
tb_init_ex(&o);

// struct with size
struct tb_init_opts o = {0};
o.size = sizeof(o);
o.file = "/dev/tty";
o.region_h = 3;
tb_init_ex(&o); // only reads up to `o.size`

// variadic [arg1_type, arg1, ...]
tb_init_ex(2,
  TB_INIT_TTY, "/dev/tty",
  TB_INIT_REGION, 3
);

// variadic [arg_spec, arg1, ...]
tb_init_ex("fr",
  "/dev/tty",
  3
);

// uintptr_t array
uintptr_t o[] = {
  TB_INIT_TTY,    (uintptr_t)"/dev/tty",
  TB_INIT_REGION, (uintptr_t)3,
};
tb_init_ex(2, &o);

// uintptr_t array 2
uintptr_t o[] = {
  TB_INIT_TTY,    (uintptr_t)"/dev/tty",
  TB_INIT_REGION, (uintptr_t)3,
  0, 0 // 0 signifies end
};
tb_init_ex(&o);

// string
tb_init_ex("file=/dev/tty,region_h=3");

[adsr]

Moved the tb_init_ex topic to its own issue #132