4 ms·

SetOrExit ("S16.Name"); Oh wait, SetOrExit is a macro ... #define SetOrExit(Name) \ if (!prop_f

SetOrExit ("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 ago
It'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 ago
  Here'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 ago
    Exceptions 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 ago
      I 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
        You are testing and returning, and responsible for propagating the error in your example, and have a chance to forget to do that. That is what IMO keeps this from being a general purpose exception. That you could correctly check the error return in one subroutine, but forget to in it's parent is the issue. Each approach has it's benefits and drawbacks. Automatically jumping to a cleanup routine means you can't forget to handle it, but also obfuscates control flow. In any case, I view it as an integral feature of exception handling.
- asynchronous13 11y ago
  Believe me, I know what it is. This just isn't the way to do it. Hiding a goto in a macro is bad. Hiding a goto in a nested macro is even worse. Sure, it's fine right now, but this is setting the project up for maintenance nightmares. 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. http://www.stroustrup.com/JSF-AV-rules.pdf http://www.stroustrup.com/JSF-AV-rules.pdf http://lars-lab.jpl.nasa.gov/JPL_Coding_Standard_C.pdf http://lars-lab.jpl.nasa.gov/JPL_Coding_Standard_C.pdf
  - retrogradeorbit 11y ago
    Just as a side note, have you looked at the systemd source code? It has many, many with gotos (and sprintfs and hard coded buffer sizes and... and...)
    - intelfx 11y ago
      Just as a side note to side note (preemptive strike against accusations of systemd for using gotos) — the Linux kernel has many of them either. This is idiomatic C-style error handling. Also note that systemd employs gcc's __attribute__((cleanup(...))) logic to improve code clarity.
    - asynchronous13 11y ago
      No, I haven't looked at systemd source code. Does it have gotos hidden in macros, too? There are many practices that used to be common that we have learned should not be common. Hopefully a new project doesn't repeat the mistakes of the past. 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. I don't think gotos are inherently bad, but I choose to heed the advice of people with more experience than me. (I do think that hiding gotos in macros is inherently bad, though.) [1] http://sdtimes.com/nasas-10-rules-developing-safety-critical-code/ http://sdtimes.com/nasas-10-rules-developing-safety-critical...

kbenson 11y ago

It'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 ago
Here'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 ago
  Exceptions 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 ago
    I 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
      You are testing and returning, and responsible for propagating the error in your example, and have a chance to forget to do that. That is what IMO keeps this from being a general purpose exception. That you could correctly check the error return in one subroutine, but forget to in it's parent is the issue. Each approach has it's benefits and drawbacks. Automatically jumping to a cleanup routine means you can't forget to handle it, but also obfuscates control flow. In any case, I view it as an integral feature of exception handling.
asynchronous13 11y ago
Believe me, I know what it is. This just isn't the way to do it. Hiding a goto in a macro is bad. Hiding a goto in a nested macro is even worse. Sure, it's fine right now, but this is setting the project up for maintenance nightmares. 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. http://www.stroustrup.com/JSF-AV-rules.pdf http://www.stroustrup.com/JSF-AV-rules.pdf http://lars-lab.jpl.nasa.gov/JPL_Coding_Standard_C.pdf http://lars-lab.jpl.nasa.gov/JPL_Coding_Standard_C.pdf
- retrogradeorbit 11y ago
  Just as a side note, have you looked at the systemd source code? It has many, many with gotos (and sprintfs and hard coded buffer sizes and... and...)
  - intelfx 11y ago
    Just as a side note to side note (preemptive strike against accusations of systemd for using gotos) — the Linux kernel has many of them either. This is idiomatic C-style error handling. Also note that systemd employs gcc's __attribute__((cleanup(...))) logic to improve code clarity.
  - asynchronous13 11y ago
    No, I haven't looked at systemd source code. Does it have gotos hidden in macros, too? There are many practices that used to be common that we have learned should not be common. Hopefully a new project doesn't repeat the mistakes of the past. 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. I don't think gotos are inherently bad, but I choose to heed the advice of people with more experience than me. (I do think that hiding gotos in macros is inherently bad, though.) [1] http://sdtimes.com/nasas-10-rules-developing-safety-critical-code/ http://sdtimes.com/nasas-10-rules-developing-safety-critical...