Bugzilla – Bug 339
Unconditional assert API proposed
Last modified: 2008-10-24 04:29:22 UTC
Looking at the common error that I made in bug 336, which Mathieu post-fixed thus: > - NS_ASSERT (bytes_read == sizeof (seeds[i])); > + if (bytes_read != sizeof (seeds[i])) > + { > + NS_FATAL_ERROR ("Read from /dev/random failed"); > + } I wonder why we do not have a macro NS_ASSERT_UNCOND (condition) which is not removed in optimized builds. #define NS_ASSERT_UNCOND(condition) if (!(condition)) { std::cerr << "Assertion `" << #condition << "' failed. Aborting." << std::endl; } It is certainly not _needed_, but it is very _convenient_. Just an idea...
I like this idea, but I would suggest calling this a conditional fatal error, not an unconditional assertion. That is, something like #define NS_FATAL_ERROR_COND(predicate, msg). Note that this would simplify almost every usage (if not _every_ usage) of NS_FATAL_ERROR, since it almost always falls in an "if" block.
To be honest I don't like NS_FATAL_ERROR_COND. ssize_t result = read (sock, &foo, N); NS_FATAL_ERROR_COND (result != N, "short read"); To me NS_FATAL_ERROR_COND makes it look like normally it produces a fatal error; not aborting feels like the exception and not the rule. Additionally, normally it is easier to specify the condition in terms of what is the expected value, rather than what values are wrong. For comparison, my original idea was: ssize_t result = read (sock, &foo, N); NS_ASSERT_UNCOND (result == N); One non obvious advantage is that it makes it easy to "upgrade" the importance of an existing assertion once the developer realizes the error check has negligible impact on performance and the error is extremely serious. Just my 0.02 €.
Assertions are designed to provide a way to check for internal program consistency. They derive from Meyer's pre- and post-condition checks. They can be compiled out for efficiency, so you never, never put any functionality in an assert, just like you never, never put any functionality in a debug statement. It's an error to do so. Having an ASSERT_UNCOND seems to blur the distinction between consistency checks and errors. Since there the idiom is, if (cond) NS_FATAL_ERROR, then it makes sense to encapsulate that in a macro to make life easier. If I were starting from scratch, I'd make an NS_ASSERT (cond) and a matching NS_FATAL_ERROR (cond) and then add an NS_ASSERT_MSG (cond, msg) and a matching NS_FATAL_ERROR_MSG (cond, msg). There are 80 instances of NS_FATAL_ERROR (msg) now, though.
(In reply to comment #3) [...] > There are 80 instances of NS_FATAL_ERROR (msg) now, though. _Please_ I beg you, do not even consider to change NS_FATAL_ERROR. Blasphemy! No more gratuitous API breakage please :| How about NS_ABORT_IF (condition, message) ? I don't think you can't get a much more explicit name than that...
(In reply to comment #4) > (In reply to comment #3) > [...] > > There are 80 instances of NS_FATAL_ERROR (msg) now, though. > > _Please_ I beg you, do not even consider to change NS_FATAL_ERROR. Blasphemy! > No more gratuitous API breakage please :| > > How about NS_ABORT_IF (condition, message) ? I don't think you can't get a > much more explicit name than that... > The fatal error and assert macros are for developer use, completely internally to the ns-3 codebase. Why are you concerned with API breakage here? I'd be surpised if a "user" even knew these macros existed. It would however be some sed work to change things. +1 to Craig's suggestion, for inclusion in ns-3.3
For me _all_ API in ns3/*.h is public. This is the type of thing that makes me want to stick to NS 3.2 for a very long time. I don't know why I care about API breakage anymore; I guess it's just reflex reaction :P
how about adding NS_FATAL_ERROR_ASSERT (cond) and NS_FATAL_ERROR_ASSERT_MSG (cond,msg) ?
The *ASSERT* macros mean to me that they are not always going to be compiled in. The FATAL_ERRROR macro means that it is always going to me compiled in. NS_FATAL_ERROR_ASSERT seems conflicted :-) I rhought about some other names, but if you leave out the ASSERT part, you tend to lose the context that ASSERT carries the sense of testing for a TRUE condition. NS_FATAL_CONDITION_MSG -- does it die if the condition is true or false? I don't think we'll do much better (conceptually) than NS_FATAL_ERROR_ASSERT although I think a shorter version is just as good: NS_FATAL_ASSERT, NS_FATAL_ASSERT_MSG Maybe the addition of ASSERT is actually better that NS_FATAL_ERROR, NS_FATAL_ERROR_MSG since they do imply a sense of the condition. But I would be fine with how adding NS_FATAL_ERROR_ASSERT (cond) and NS_FATAL_ERROR_ASSERT_MSG (cond,msg).
I had already suggested "NS_ABORT_IF (condition, message)". Or maybe "NS_FAIL_UNLESS (condition, message)". I think these make it very clear what the macro does by its very name, more than any other alternative.
NS_ABORT_IF (condition) NS_ABORT_IF_MSG (condition, message) This is fine by me.
(In reply to comment #10) > NS_ABORT_IF (condition) > NS_ABORT_IF_MSG (condition, message) > > This is fine by me. > +1
Created attachment 267 [details] proposed patch changed NS_ABORT_IF_MSG to NS_ABORT_MSG_IF to allow the variant NS_ABORT_MSG_IF_NOT which feels better than NS_ABORT_IF_NOT_MSG
Maybe UNLESS instead of IF_NOT?
(In reply to comment #13) > Maybe UNLESS instead of IF_NOT? ok
changeset 0eea20a7b592