More from Mark and Barry on Assert

| 9 Comments

Mark responded to Barry's response to my post on Assert. Barry then responded to Mark and Kim and I added a little more in the comments. I did have some more to say, but Barry's said most of it...

Anyway, so far only Vagn Johansen and the guru's over on comp.lang.c++.moderated disagree with us... Anyone else fancy joining in? What I'd like to see is for someone who's defending to show a situation where the alternatives that we're proposing don't work; but lets try and stay out of the realms of "what if the compiler is broken" and "what if there's a hardware fault" if we can...

[Updated 3rd October]
See the following follow up postings:

9 Comments

Just posting my opinions (and practices) on asserts. I'm not hoping or wanting to start a good old-fashioned flame-war :-)

As stated I don't see that assertions, these days, give you anything you can't get from other practices, process or tools.

If you get a hardware fault on the machine, odds are that everything else will be affected to, so diagnosing it will be pretty obvious anyway. And that's what Sun engineers/SAs are paid to do, and there's a whole infrastructure built by the vendor themselves to deal with it. Oh, and on Windows Server 2003 et al. as well.

As for broken compilers, well, any decent developer worth his salt should know about the faults of the compiler they work with or at the very least should know how much of the ANSI standard it actually implements. Either way, debugging is often the only way to figure out that problem .

Can Barry make up his mind?

He writes "Assertions are still evil" and
"My issue with assertions is in 68.27% of cases, ...".

Why not 100%?

Lies, damn lies, and statistics. The other 32% (or so), are either even worse, or have a good reason. So far no-one has posted a good reason. I'd like to see an example to that effect.

As I said, this is my take on assertions, and the way I code.



UINT32 Buffer::DecRefCnt()
{
return ::InterlockedDecrement( &m_RefCnt );
};

Suppose I consider adding an ASSERT(m_RefCnt> 0) before the decrement then what should I do instead?

Something like this?

    UINT32 Buffer::DecRefCnt()
    {
       if (m_RefCnt == 0)
       {
          throw MyException("Buffer::DecRefCnt()", "Reference count is zero");
       }
  
       return ::InterlockedDecrement(&m_RefCnt);
    }

however that's not thread safe, and I assume you care because you're using InterlockedDecrement() so I'd probably end up with something like this:


UINT32 Buffer::DecRefCnt()
{
const UINT32 result = ::InterlockedDecrement(&m_RefCnt);

if (result == MAXUINT32) // or if (result + 1 == 0)
{
throw MyException("Buffer::DecRefCnt()", "Reference count decremented from zero");
}

return result;
}

As this way you're sure not to miss the zero... by having two threads pass the check when m_RefCnt is 1 and having both decrement atomically and end up with -1...

Same answer as Len (the easy option) - throw an exception as it's an 'exceptional' situation. I'd also ask why you have not chosen to factor the reference counting into some (templated) base class, and perhaps handle it there at a single point. That's another cause of lazy programming, is repeating yourself (as in don't repeat yourself aka DRY), many things can and should be handled once. If the full piece of code is decrementing all over the place I'd certainly look at fixing that before I'd *even* consider asserting preconditions.

The manual reference counting is because I wanted total control/knowledge of what happened in the code for performance reasons. It is very rare that I do this and normally I use either boost::shared_ptr or boost::scoped_ptr.

Who is going to catch MyException? Do you update the documentation for the function saying that it can throw MyException? Do you expect the caller to know about / handle it?

VJ,

Yes, I'd update the documentation to state that an exception may be thrown if DecRefCnt() is called on a buffer that has an reference count of zero. Of course it depends on what you do with the buffer when the reference count goes to zero as to how useful this check will actually be... If you've deleted the object then the user may have already had to access the object via an invalid pointer and so may not have even got this far. However, it may still be of some help in some multi-threaded race condition situations so it's probably worth having...

As to who catches it, well, it's the kind of exception that you can't really recover from. So I would catch it at the thread or process boundary, log that it happened and initiate a shutdown.

Note that the obvious, simple, single line, assert added the the start of the function wont work due to the threading issue and that the problem it's likely checking for is probably more likely to occur in the release build than in the debug build; race conditions are like that.

Leave a comment