Removed gotos from main.c

[ghost]

Small chunks are good.

[hzeller]

Nice, only one little comment.

[ghost]

Slightly off topic: I have an impression that the original author has a Visual Basic (V3.x?) background.

In VB 'true' was -1 and 'false' was 0. Furthermore, it was very common to terminate functions with a labeled return statement in the end of the function, using goto's to get to the label.

I could be wrong, but to me this is how it looks.

[hzeller]

I don't know anything about Visual Basic, so I can't comment on that.

-1 being TRUE and 0 being FALSE sounds like something potentially reasonable, as -1 essentially means that all bits in the underlying storage are set (in a signed byte, e.g. -1 would be 11111111).

Here we see the opposite: 0 being 'OK', while negative return value means 'something is wrong'. This actually is pretty common practice in all system-like function calls. Typically 0 or positive return values mean success there, and negative return values (or just -1) failure. That is rooted in the fact that there is only one successful outcome, but potentially many different errors. The upnp library we're using for instance does that as well (0 means success, then there is a bunch of negative return codes for different error conditions).

Having said that, for this code, when we have to deal with 'success' or 'failure', it would be better to just return TRUE (something != 0) and FALSE (=0). Which is what you started doing.

As for 'goto's: they are as well pretty common in C code (not C++ code). Typically, the reason is to help cleanup resources, that have been acquired and need to be cleaned up (memory to free, mutex to unlock...). Instead of doing that in every early return statement, this is only done once at the end.

void fun() { char *a = malloc(something);

// do something if (error_condition) goto out;

// more stuff. At the end, don't need 'a' anymore.

out: free(a); return; }

Now, it is only really useful if there is actually something to clean up. That happens in gmrender, but is rare, e.g. look at upnp_device_init()). In the case you were working on, it is perfectly ok (and more readable) to just do an early return.

For instance, we would need a goto in main(), if we had something between upnp_device_init() and upnp_device_shutdown() that could fail; then we would have to have a label before that exit (shutting down the device is necessary to tell connected clients about the departure).

So, no matter what Dijkstra says in other circumstances - in these resource cleanup cases, 'goto' is actually not harmful but useful and considered accepted and 'good' practice. It is a common pattern in C.

You won't see this in C++ code, because there are ways to automatically clean up resources (RAII).

For all functions that don't have to clean up anything, we don't need the gotos, I am in favor of removing the gotos; I suspect, the original owners used some kind of code generation framework, that set this up 'just in case', so that it is simple to add this pattern.