I'm going to disagree with you on the "if ((error = ...) != success) goto fail;" pattern. I've written a lot of C/C++ during my career, but mostly C# in the last ten years, and that pattern looks totally obvious to me. This isn't because I'm super-smart (I'm not) but because I've internalised the language rules and common patterns and I don't have to think hard about what such a code line is doing. It just seems like a normal c/c++ construct. I understand that it might be more opaque to a ruby/scala/whatever dev, but they probably wouldn't be poking around in libssl unless they knew C/C++.
To me, wrapping such a simple test in a macro just obscures what is going on by introducing unnecessary abstraction. Making the decision about whether to use a macro in this case seems to me rather like the decision to use operator overloading in C++. There are occasions when you might want to do it to hide complexity, but you'd better have a good reason.
The issue here isn't that the code is confusing or somehow not "obvious" in function: the issue is that there are numerous ways we can mistype it, especially as we (apparently) are prone to copy/pasting it because it involves typing an additional 30 keystrokes (in this case, assuming "err" and "0"). The goal isn't to "hide complexity", the goal is to "remove things the developer can do wrong".
Trust me: I glance at that code, and it is 100% clear to me in that moment what it is doing and why it is doing it--I have been programming primarily in C/C++ now for almost 20 years--but I simply do not believe you that an experienced C/C++ programmer glances at this code and knows 100% for certain that it is correct. If nothing else, clearly the person who wrote this code screwed it up ;P.
This pattern just lends itself to silly mistakes: you might check the error using the wrong constant (!= 0 when you need == -1; this could even work temporarily!) or think that a function can't return an error when it actually can (the setuid mistake that burned Android, see Rage Against the Cage); by using structured error handling you aren't just "hiding" complexity: you are removing it.
Now, I happily admit (and already did in my original comment) that the macro doesn't solve all of these problems as well as "use a language that provides better abstractions (even C++!)"; however, it does mean that you can't accidentally miss the set of parentheses (assigning the comparison result), double up the equal sign (comparing instead of assigning), add an extra semicolon (breaking the comparison, so as to always "goto fail;"), copy/paste the boilerplate incorrectly (which you might have even done in a misguided attempt to avoid the previous errors ;P) leaving you with a second copy of "goto fail;" (as the developer here probably did; it might also have been a merge failure: thankfully, that also becomes impossible, as the error check now is naturally part of the same line as the code being checked), or otherwise make any silly "it still compiles, it looks almost identical, but now it doesn't work" errors. That's valuable :/.
"but I simply do not believe you that an experienced C/C++ programmer glances at this code and knows 100% for certain that it is correct."
I agree, but then I didn't actually say that I would know it was correct (to any percentage) by glancing at it. What I tried to say was that the "boilerplate" does not detract from the code exposing its meaning or intention.
Again, I agree that the the wrong error constant could be used. But thats also true of a macro is used - especially if the constant is embedded in the macro which is elsewhere.
I didn't want to get into your point about whether this would be better written in another language, because we are where we are. libssl has bee around for a while and has accumulated plenty of dependencies. I don't think a re-write is going to happen any time soon.
You seem to have ignored the long list of "possible typos similar to this one" that were my arguments for why the macro was better than having the developer copy/paste the code every time. The only argument you seem to be making is: "abstraction is bad"; I mean, this same argument (that abstraction hides functionality and values behind opaque names) is also the reason why developers should not write functions...
Comments
I'm going to disagree with you on the "if ((error = ...) != success) goto fail;" pattern. I've written a lot of C/C++ during my career, but mostly C# in the last ten years, and that pattern looks totally obvious to me. This isn't because I'm super-smart (I'm not) but because I've internalised the language rules and common patterns and I don't have to think hard about what such a code line is doing. It just seems like a normal c/c++ construct. I understand that it might be more opaque to a ruby/scala/whatever dev, but they probably wouldn't be poking around in libssl unless they knew C/C++.
To me, wrapping such a simple test in a macro just obscures what is going on by introducing unnecessary abstraction. Making the decision about whether to use a macro in this case seems to me rather like the decision to use operator overloading in C++. There are occasions when you might want to do it to hide complexity, but you'd better have a good reason.
The issue here isn't that the code is confusing or somehow not "obvious" in function: the issue is that there are numerous ways we can mistype it, especially as we (apparently) are prone to copy/pasting it because it involves typing an additional 30 keystrokes (in this case, assuming "err" and "0"). The goal isn't to "hide complexity", the goal is to "remove things the developer can do wrong".
Trust me: I glance at that code, and it is 100% clear to me in that moment what it is doing and why it is doing it--I have been programming primarily in C/C++ now for almost 20 years--but I simply do not believe you that an experienced C/C++ programmer glances at this code and knows 100% for certain that it is correct. If nothing else, clearly the person who wrote this code screwed it up ;P.
This pattern just lends itself to silly mistakes: you might check the error using the wrong constant (!= 0 when you need == -1; this could even work temporarily!) or think that a function can't return an error when it actually can (the setuid mistake that burned Android, see Rage Against the Cage); by using structured error handling you aren't just "hiding" complexity: you are removing it.
Now, I happily admit (and already did in my original comment) that the macro doesn't solve all of these problems as well as "use a language that provides better abstractions (even C++!)"; however, it does mean that you can't accidentally miss the set of parentheses (assigning the comparison result), double up the equal sign (comparing instead of assigning), add an extra semicolon (breaking the comparison, so as to always "goto fail;"), copy/paste the boilerplate incorrectly (which you might have even done in a misguided attempt to avoid the previous errors ;P) leaving you with a second copy of "goto fail;" (as the developer here probably did; it might also have been a merge failure: thankfully, that also becomes impossible, as the error check now is naturally part of the same line as the code being checked), or otherwise make any silly "it still compiles, it looks almost identical, but now it doesn't work" errors. That's valuable :/.
"but I simply do not believe you that an experienced C/C++ programmer glances at this code and knows 100% for certain that it is correct."
I agree, but then I didn't actually say that I would know it was correct (to any percentage) by glancing at it. What I tried to say was that the "boilerplate" does not detract from the code exposing its meaning or intention.
Again, I agree that the the wrong error constant could be used. But thats also true of a macro is used - especially if the constant is embedded in the macro which is elsewhere.
I didn't want to get into your point about whether this would be better written in another language, because we are where we are. libssl has bee around for a while and has accumulated plenty of dependencies. I don't think a re-write is going to happen any time soon.
You seem to have ignored the long list of "possible typos similar to this one" that were my arguments for why the macro was better than having the developer copy/paste the code every time. The only argument you seem to be making is: "abstraction is bad"; I mean, this same argument (that abstraction hides functionality and values behind opaque names) is also the reason why developers should not write functions...