Boost logo

Boost :

From: williamkempf_at_[hidden]
Date: 2001-04-18 09:08:35

--- In boost_at_y..., Jesse Jones <jesjones_at_h...> wrote:
> >I've uploaded an updated header and a test file.
> I just looked it over and I have some comments for you:
> 1) I think it'd be a good idea if Stack tested to see if nested
> checks work properly. For example:
> int& top()
> {
> return _stack[_top];
> }
> void push(int i)
> {
> BOOST_REQUIRE(_top < _capacity);
> BOOST_OBSERVE(unsigned, _top);
> do_push(i);
> BOOST_ENSURE(BOOST_OLD(_top) == _top - 1);
> BOOST_ENSURE(_stack[_top - 1] == i);
> }
> private:
> void do_push(int i)
> {
> // break the invariant
> unsigned old_cap = _capacity;
> _capacity = 0;
> // call a public method: invariant should not fire
> top() = i;
> ++_top;
> // restore the invariant
> _capacity = old_cap;
> }

Thanks. I'll make this change. Note, however, that this code is
proof of concept not production code. It's not been thoroughly
tested. I expect there are several bugs lurking about. I also
expect that the interface is far from being acceptable. I presented
it more to be a basis for discussion on what such a library needs to
include, what issues need to be addressed, and how best to define the
interfaces. Don't expect this to be anything like the final library
we decide on.

> 2) If you make this change you'll see that the check_invariants
> function is buggy. It should look more like this:
> public: inline void check_invariants(bool entering) const
{ \
> unsigned& count = _boost_invariant_count.get(); \
> if (entering) { if (++count > 1) return; } \
> if (!entering) { if (--count > 0) return; }
> (I also switched the argument to a bool, but that's no big deal).

I've used an int an purpose, though it's not appearant from the
current implementation. Invariant checks in the c-tor and d-tor have
an ugly side effect of needlessly checking the invariants twice. The
int will allow refinements that will prevent this. Other wise, yes
you're correct in your assessment here.
> 3) The check_invariants class is overly complex and does heap
> allocations which is pretty nasty. You can fix that by using that
> voodoo I mentioned earlier:
> class check_invariants {
> public:
> template <typename T>
> check_invariants(const T* object) {
> _object = object;
> _invoker = invoke<T>;
> _invoker(_object, true);
> }
> ~check_invariants() {
> _invoker(_object, false);
> }
> template <class T>
> static void invoke(const void* object, bool entering) {
> void (T::*method)(bool) const = &T::check_invariants;
> (static_cast<const T*>(object)->*method)(entering);
> }
> private:
> void (*_invoker)(const void*, bool);
> const void* _object;
> };

Actually, this is at least as ugly ;). I didn't care for the memory
allocation, however, and this solution is nicer in this regard.
> 4) It'd be nice if main printed a message for the expected errors.
> You might also want to wrap all of main in a try/catch block to
> any unexpected errors.

Again, this is not code meant for final consumption. I guess I
shouldn't have called main.cpp a test, because it gives the
impression that it's a full unit test framework, and it's not. It's
a simple piece of code that you can use to play around with the
concepts and see what happens in your debugger. I'd agree that
better tests are needed, but right now we just have a toy and I
didn't feel compelled to write a lot of throw away test code.

> 5) The code is setup so that the invariant is always an inline
> in the header. I really don't like this: invariants are often too
> to be good candidates for inlining and I don't want to touch a
> every time I change an invariant.
> One possible solution is to get rid of BOOST_BEGIN_INVARIANTS
> mutable boost::invariant_count _boost_invariant_count; \
> void check_invariants(bool entering) const { \
> unsigned& count = _boost_invariant_count.get(); \
> if (entering) { if (++count > 1) return; } \
> if (!entering) { if (--count > 0) return; } \
> this->invariant(); \
> }
> In the header clients have to add " BOOST_SETUP_INVARAINT;" and
> declare the invariant method. In the cpp they define the invariant
> method as a normal method and use BOOST_INVARIANT to do the checks.

If you're familiar with MFC there are better constructs:

   mutable boost::invariat_count _boost_invariant_count;
   void check_invariants(bool entering) const;

#define BOOST_BEGIN_INVARIANTS(class_name) \
   void class_name::check_invariants(bool entering) const { \
      unsigned& count = _boost_invariant_count.get(); \
      if (entering) { if (++count > 1) return; } \
      if (!entering) { if (--count > 0) return; }


I say this is better because we pollute the class name space with
only one method name (and I'd obscure this name in some way for
production code, like maybe appending a GUID to the name). I
originally chose to make things inline only to minimize the number of
macros used... I expect many will be uncomfortable with the heavy use
of macros here, and probably rightfully so. You are right that
putting this in the header can cause problems during development,

Bill Kempf

Boost list run by bdawes at, gregod at, cpdaniel at, john at