Props to the author for posting the "testimonials." Got a chuckle out of it. Did not look at the code.
I'd be happy giving it to a new programmer to use as a style guide. I have no idea if the architecture is suitable for the purpose, but anyone who is dismissing this based on code quality should be ignored.
Representative Sample: https://github.com/ServiceManager/ServiceManager/blob/master...
#define DbgEnteringState(x) \
printf ("[%s] Unit entering state %s\n", unit->name, #x);
static void dbg_entering_state(unit_t * unit, const char * state_name) {
fprintf (stderr, "[%s] Unit entering state %s\n", unit->name, state_name);
}
SetOrExit ("S16.Name");
#define SetOrExit(Name) \
if (!prop_find_name (new_svc->properties, Name)) \
{ \
OnError ("error: %s not set", #Name); \
}
#define OnError(...) \
fprintf (stderr, __VA_ARGS__); \
goto on_error;
It does not use macros or goto and I find that it makes the code easy to reason locally about.
In summary:
struct RefreshDevices {
// fields that require clean up
}
static void deinit_refresh_devices(struct RefreshDevices *rd) {
// clean up rd regardless of what state it's in
}
static int refresh_devices(struct SoundIoPrivate *si) {
struct RefreshDevices rd = {0};
if (error_occurred) {
deinit_refresh_devices(&rd);
return ErrorCode;
}
// ...
deinit_refresh_devices(&rd);
return 0;
}
If calling deinit on every exit path of the function is too much typing you can wrap that function in another function that calls deinit, and the function doing the actual work can just return an error code without worrying about cleanup.
What's the benefit of this way over a more explicit coding style? The only benefit is saving a few lines of typing. Code is typed once and read 1000's of times. It's better to optimize code for reading, not for writing.
Feel free to read the Joint Strike Fighter (JSF) coding standards which says "Goto shall not be used", and "Macros shall not be used, inline functions are preferred". Also, see NASA's Joint Propulsion Labs (JPL) coding standards where they recommend against using goto, and only allow simple macros (hint: a goto inside a macro is not simple).
Some really smart people put a lot of effort into creating coding standards for critical systems. Even if you don't believe me, when Bjarne Stroustrup is hosting the coding standard on his personal website you might want to pay attention.
Also note that systemd employs gcc's __attribute__((cleanup(...))) logic to improve code clarity.
From earlier this year, NASA's 10 rules for safety critical systems [1]:
Rule 1: Restrict all code to very simple control flow constructs.
Do not use GOTO statements, setjmp or longjmp constructs, or
direct or indirect recursion.
[1] http://sdtimes.com/nasas-10-rules-developing-safety-critical...
On the other hand no other engineering will be asked to change the requirement and spec hundreds of time during its lifetime either since that would be impossible to achieve.
2. the return value of malloc() et al. isn't checked (http://www.etalabs.net/overcommit.html)
3. the register storage-class specifier is used multiple times (mainly lib/s16db/translate.c)
4. function declarations are missing a prototype (() instead of (void))
I (and likely the author) disagree with you on the importance of checking the return value of malloc(). Because of overcommit (as you point out), a non-null value returned from malloc() does not mean that you will not crash when you access the pointer. If the pointer is used directly, crash on NULL might a reasonable approach. It's when NULL is retained and then used as the base of an array that it may become a security problem. Configuring malloc() to abort() on failure would be probably my preferred solution.
I agree with the last point, but think it's a minor one. While I'd like C to treat () in a function definition as equivalent to (void), for historical reasons it does not. The author trades off the visual noise of the word 'void' for better error reporting. But depending on your compiler, you may still get a warning. On the computer I just tried, 'icc' and 'clang' gave clear warnings but 'gcc' did not.
nate@ubuntu:~/C$ cat void.c
#include <stdio.h>
int empty() { return 3; }
int main(void) { printf("%d\n", empty(4)); return 0; }
nate@ubuntu:~/C$ icc -Wall -o void void.c
void.c(3): warning #140: too many arguments in function call
int main(void) { printf("%d\n", empty(4)); return 0; }
^
nate@ubuntu:~/C$ clang -Wall -o void void.c
void.c:3:40: warning: too many arguments in call to 'empty'
int main(void) { printf("%d\n", empty(4)); return 0; }
~~~~~ ^See the experience of the D-Bus (which tries hard to be OOM-safe) on this topic: http://blog.ometer.com/2008/02/04/out-of-memory-handling-d-b...