5 ms·

I decided to see if there was any merit to your snarky comment: $ git clone git://anongit.freedesktop.org/systemd/systemd systemd-git $ cd systemd-git

I decided to see if there was any merit to your snarky comment: $ git clone git://anongit.freedesktop.org/systemd/systemd systemd-git $ cd systemd-git $ grep -r "assert(" | wc -l 7771 That looks pretty bad at first. But I noticed this in src/shared/macro.h: /* We override the glibc assert() here. */ #undef assert #ifdef NDEBUG #define assert(expr) do {} while(false) #else #define assert(expr) assert_se(expr) #endif I didn't verify that macro.h was included by every file that used assert(). But I'm assuming it does. So it looks like assert() at least doesn't call abort() in their world. In fact, it's a nop in non-debug builds. And even in debug builds, the assert_se() macro logs a message and continues. I didn't check for any external libraries systemd might use.

vidarh 12y ago
Also, the systemd repository contains dozens of other programs than systemd itself. Even if any of those were to abort, the implications for systemd would be far less severe. Not nearly all the git code will ever be running in pid 1.
unwind 12y ago
This would be a great place to insert a snarky comment about the sanity of overloading a standard function. I believe it violates the principle of least surprise, which I generally would consider a good principle. As a new reader of the code, you'd be quite sane to expect "assert" to mean assert(), not "our local assert(), which is something different". There should be a --disallow-keyword-redefinitions flag to the preprocessor, or something. Grumble.
- GFK_of_xmaspast 12y ago
  The "principle of least surprise" would say that of course asserts are turned off with -NDEBUG. From assert(3) on osx: """The assert() macro may be removed at compile time with the cc(1) option -DNDEBUG."""
- tinco 12y ago
  'assert' is not a keyword. Even if it were, they overrode it with a function that has the exact same signature and general runtime behaviour. Hardly anything to be surprised about. Also, invoking the principle of least surprise on a C codebase is just ludicrous.
  - unwind 12y ago
    I didn't say it was a keyword. It's a standard function-like macro (not a function, as I wrote). I would think it just as bad an idea to overload `NULL` with something non-standard. The standard things in the language are something to be learned, and they should remain constant and not change per-project just because someone can do it differently. Do it however you like, but don't clobber the standard names, is all I'm saying.
- abecedarius 12y ago
  Yes, when I want a nonstandard assert() I normally give it a different name, like insist(). No reason not to.
- skj 12y ago
  "The principle of least surprise" is, in every use I've ever seen, just an obnoxious way to say that it's not how you would have done it.
- __david__ 12y ago
  In general I would agree with you—it sucks to override a standard function with weird, local, non-standard functionality. However, in this case, it's an extremely common practice to override assert() so that it only aborts or warns during testing or when some DEBUG macro is defined. To attack that is to ignore long-standing, wide-spread idioms.

vidarh 12y ago

Also, the systemd repository contains dozens of other programs than systemd itself. Even if any of those were to abort, the implications for systemd would be far less severe. Not nearly all the git code will ever be running in pid 1.

unwind 12y ago

This would be a great place to insert a snarky comment about the sanity of overloading a standard function. I believe it violates the principle of least surprise, which I generally would consider a good principle. As a new reader of the code, you'd be quite sane to expect "assert" to mean assert(), not "our local assert(), which is something different". There should be a --disallow-keyword-redefinitions flag to the preprocessor, or something. Grumble.

GFK_of_xmaspast 12y ago
The "principle of least surprise" would say that of course asserts are turned off with -NDEBUG. From assert(3) on osx: """The assert() macro may be removed at compile time with the cc(1) option -DNDEBUG."""
tinco 12y ago
'assert' is not a keyword. Even if it were, they overrode it with a function that has the exact same signature and general runtime behaviour. Hardly anything to be surprised about. Also, invoking the principle of least surprise on a C codebase is just ludicrous.
- unwind 12y ago
  I didn't say it was a keyword. It's a standard function-like macro (not a function, as I wrote). I would think it just as bad an idea to overload `NULL` with something non-standard. The standard things in the language are something to be learned, and they should remain constant and not change per-project just because someone can do it differently. Do it however you like, but don't clobber the standard names, is all I'm saying.
abecedarius 12y ago
Yes, when I want a nonstandard assert() I normally give it a different name, like insist(). No reason not to.
skj 12y ago
"The principle of least surprise" is, in every use I've ever seen, just an obnoxious way to say that it's not how you would have done it.
__david__ 12y ago
In general I would agree with you—it sucks to override a standard function with weird, local, non-standard functionality. However, in this case, it's an extremely common practice to override assert() so that it only aborts or warns during testing or when some DEBUG macro is defined. To attack that is to ignore long-standing, wide-spread idioms.