Re: Boost.Assertion

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()
>      {
>          BOOST_CHECK_INVARIANTS();
>          return _stack[_top];
>      }
> 
>      void push(int i)
>      {
>          BOOST_REQUIRE(_top < _capacity);
>          BOOST_CHECK_INVARIANTS();
>          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 
catch 
> 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 
method 
> in the header. I really don't like this: invariants are often too 
big 
> to be good candidates for inlining and I don't want to touch a 
header 
> every time I change an invariant.
> 
> One possible solution is to get rid of BOOST_BEGIN_INVARIANTS 
> /BOOST_END_INVARIANTS, and add BOOST_SETUP_INVARAINT:
>      #define BOOST_SETUP_INVARAINT    \
>          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:

#define BOOST_DECLARE_INVARIANTS \
   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; }

#define BOOST_END_INVARIANTS() \
   }

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, 
though.

Bill Kempf