5 ms·
I agree with the comment about overusing macros though. Instead of: #define DbgEnteringState(x) \
by AndyKelley 11y ago
I agree with the comment about overusing macros though. Instead of:
#define DbgEnteringState(x) \
printf ("[%s] Unit entering state %s\n", unit->name, #x);
How about:
static void dbg_entering_state(unit_t * unit, const char * state_name) {
fprintf (stderr, "[%s] Unit entering state %s\n", unit->name, state_name);
}
Friendlier to humans reading code, IDEs, and debuggers. I went ahead and fixed it writing to the wrong stdio stream too.
- bcoates 11y agoThat's clearly trace code intended to be defined away by the build system. It's idiomatic to make such things macros. Also, your way forces unnecessary verbosity onto the caller; passing unit by name was not a typo.
- asynchronous13 11y agoSetOrExit ("S16.Name"); Oh wait, SetOrExit is a macro ... #define SetOrExit(Name) \ if (!prop_find_name (new_svc->properties, Name)) \ { \ OnError ("error: %s not set", #Name); \ } Oh wait, OnError is also a macro #define OnError(...) \ fprintf (stderr, __VA_ARGS__); \ goto on_error; There's a goto hidden in a doubly-nested macro? Sorry, that is not good practice.
- kbenson 11y agoIt's C style exception handling. Both the two macros, the place they are used, and the jump target all exist within the same 80 line file. I don't even write C and it's pretty obvious what's going on here. I might choose somewhat different names for the macros, but I'm not going to argue too much about that, it's a slippery slope.
- AndyKelley 11y agoHere's an alternate method of C exception handling: https://github.com/andrewrk/libsoundio/blob/4ce3429bdd9b08a01cb8f1b60c38a61dc5d818df/src/coreaudio.cpp#L411 https://github.com/andrewrk/libsoundio/blob/4ce3429bdd9b08a0... 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; } Whenever the ownership of a resource changes to something other than the stack of refresh_devices, you just null it out and it won't get cleaned up. 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.
- kbenson 11y agoExceptions as a programming language concept often include a change in control flow. While your method does "handle and exceptional situation", I don't think it could be said to be a general purpose way of doing exception handling in C, if you want to support the concept of changing control flow. While I recognize that you yourself may or may not have valid arguments about whether altering the control flow is a good idea, I don't think that affects whether this works as a solution for exception handling, in the general, PL concept sense.
- AndyKelley 11y agoI don't follow. You are saying that returning from a function early is not "changing the control flow"? What exactly is your criteria for "solution to exception handling"?
- kbenson 11y ago