I have written a smart pointer class called ptr<X>
Here are two ways it could be used:

void foo(ptr<X> p)
{
     // use p here
}

void foo(const ptr<X>& p)
{
    ASSERT(&p != null);
    // use p here
}

Based on my work it seems that boths ways are equally efficient
because the size of the ptr<X> class is 4 bytes (enough to hold a
pointer variable) and therefore the variable can be passed inside a
register.  Therefore it makes sense to use the first way because it is
simpler.  The following code is the source code for my ptr<X> template
class:

template<class X>
class ptr
{
private:
    X* inner_ptr;

public:
    ptr()
    {
       inner_ptr = null;
    }

    ptr(const ptr& p)
    {
       ASSERT(&p != null);
       inner_ptr = p.inner_ptr;
       if (inner_ptr != null)
       {
          inner_ptr->ref_count++;
       }
    }

    ptr& operator = (const ptr& p)
    {
       ASSERT(this != null);
       ASSERT(&p != null);
       if (p.inner_ptr != null) // protect against s = s
       {
          p.inner_ptr->ref_count++;
       }
       if (inner_ptr != null)
       {
          assert(inner_ptr->ref_count > 0); // protect against double
deletions
          inner_ptr->ref_count--;
          if (inner_ptr->ref_count <= 0)  // protect against case < 0
          {
             inner_ptr->ref_count = 1; // protect against internal
deletions
             delete inner_ptr;
          }
       }
       inner_ptr = p.inner_ptr;
       return *this;
    }

    ~ptr()
    {
       ASSERT(this != null);
       if (inner_ptr != null)
       {
          assert(inner_ptr->ref_count > 0); // protect against double
deletions
          inner_ptr->ref_count--;
          if (inner_ptr->ref_count <= 0) // protect against case < 0
          {
             inner_ptr->ref_count = 1; // protect against internal
deletions
             delete inner_ptr;
          }
#ifndef NDEBUG
          inner_ptr = null;
#endif /* NDEBUG */
       }
    }

    ptr(X* x)
    {
       inner_ptr = x;
       if (inner_ptr != null)
       {
          inner_ptr->ref_count++;
       }
    }

    ptr& operator = (X* x)
    {
       ASSERT(this != null);
       if (inner_ptr != null)
       {
          assert(inner_ptr->ref_count > 0); // protect against double
deletions
          inner_ptr->ref_count--;
          if (inner_ptr->ref_count <= 0) // protect against case < 0
          {
             inner_ptr->ref_count = 1; // protect against internal
deletions
             delete inner_ptr;
          }
       }
       inner_ptr = x;
       if (inner_ptr != null)
       {
          inner_ptr->ref_count++;
       }
       return *this;
    }

    ///
    /// NOTE: Be careful not to store this pointer in a long-term
    /// variable or else it will wreck the auto-GC system...
    /// (Therefore the Lisp AutoGC system should search for such
pointers)
    ///
    /// NOTE: fails if pointer is null, if null is OK, use method
get_ptr()
    ///
    X* operator -> () const
    {
       ASSERT(this      != null);
       ASSERT(inner_ptr != null);
       return inner_ptr;
    }

    X& operator * () const
    {
       ASSERT(this      != null);
       ASSERT(inner_ptr != null);
       return *inner_ptr;
    }


    X* get_ptr() const
    {
       ASSERT(this      != null);
       return inner_ptr;
    }

    ///
    /// NOTE: these methods are like Lisp's eq function (not Lisp's
equal function)
    ///
    /// NOTE: g++ does not optimise these functions very well...
    ///
    friend bool operator == (const ptr& p1, const ptr& p2)
    {
       if ((&p1 == null) && (&p2 == null))
       {
          return true;
       }
       else if ((&p1 == null) || (&p2 == null))
       {
          return false;
       }
       else
       {
          return (p1.inner_ptr == p2.inner_ptr);
       }
    }
    friend bool operator != (const ptr& p1, const ptr& p2)
    {
       return !(p1 == p2);
    }
};


-- 
      [ See http://www.gotw.ca/resources/clcm.htm for info about ]
      [ comp.lang.c++.moderated.    First time posters: Do this! ]

Davin Pearson wrote:
> I have written a smart pointer class called ptr<X>
> Here are two ways it could be used:
>
> void foo(ptr<X> p)
> {
>      // use p here
> }
>
> void foo(const ptr<X>& p)
> {
>     ASSERT(&p != null);
>     // use p here
> }

Um, why that assertion? If you have a reference, and taking the address of
that reference yields NULL, you have undefined behaviour (NULL pointer
dereference) already anyway!

> Based on my work it seems that boths ways are equally efficient
> because the size of the ptr<X> class is 4 bytes (enough to hold a
> pointer variable) and therefore the variable can be passed inside a
> register.  Therefore it makes sense to use the first way because it is
> simpler.

The copying overhead should be small anyway. The question is what it should
mean, e.g. passing a shared_ptr or an auto_ptr it would mean different
things.


> The following code is the source code for my ptr<X> template
> class:
>
> template<class X>
> class ptr
> {
> private:
>     X* inner_ptr;
>
> public:
>     ptr()
>     {
>        inner_ptr = null;
>     }

- Members of a 'class' are private by default, no need to repeat it.
- Where is the definition of 'null'?  BTW, ASSERT() is also not standard, it
is called assert() there!
- You should use initialisation instead of assignment.

>     ptr& operator = (const ptr& p)
>     {
>        ASSERT(this != null);
>        ASSERT(&p != null);
>        if (p.inner_ptr != null) // protect against s = s
>        {
>           p.inner_ptr->ref_count++;
>        }
>        if (inner_ptr != null)
>        {
>           assert(inner_ptr->ref_count > 0); // protect against
>                                                double deletions
>           inner_ptr->ref_count--;
>           if (inner_ptr->ref_count <= 0)  // protect against case < 0
>           {
>              inner_ptr->ref_count = 1; // protect against
>                                           internal deletions
>              delete inner_ptr;
>           }
>        }
>        inner_ptr = p.inner_ptr;
>        return *this;
>     }

- Check for self-assignment is typically done like 'if(this!=&other)'.
- You could have written this much easier if you had just created a
temporary copy of 'p' and then swapped it with the asignee.
- I don't understand what you mean with 'internal deletions' and why you set
the refcouter to one again.
- I don't understand why you are checking for a negative refcount. Rather,
you should assert that this doesn't happen. You will have to use different
means than the increment/decrement operators when used in a multithreaded
environment tohugh.
- Prefer prefix increment to postfix increment.

>     ptr(X* x)
>     {
>        inner_ptr = x;
>        if (inner_ptr != null)
>        {
>           inner_ptr->ref_count++;
>        }
>     }

Consider making this one explicit to avoid implicit conversions from a
pointer to a ptr<X>. The problem occurs when you change a function:

  void foo(X* px);

to

  void foo( ptr<X> px);

If you used to call it like this:

  X x;
  foo(&x);

this will still compile but suddenly you have a ptr<X> taking ownership
of 'x' and deleting it when foo() returns. An explicit constructor makes it
mandatory to take according actions, i.e. increment the refcounter in
advance so that the ptr<X> doesn't reach zero.

>     ptr& operator = (X* x)
>     {[...]}

Same as other assignment operator overload: use a temporary and swap with
it. Obviously, you will need a swap function for that:

  friend void swap( ptr& p1, ptr& p2) {
    std::swap( p1.inner_ptr, p2.inner_ptr);
  }

>     friend bool operator == (const ptr& p1, const ptr& p2)
>     {
>        if ((&p1 == null) && (&p2 == null))
>        {
>           return true;
>        }
>        else if ((&p1 == null) || (&p2 == null))
>        {
>           return false;
>        }
>        else
>        {
>           return (p1.inner_ptr == p2.inner_ptr);
>        }
>     }

You are checking the addresses of the ptr<X> objects here, but those must
not be null as explained above! The only thing this function should do is

   return p1.inner_ptr == p2.inner_ptr;

...and then I'm pretty sure that GCC will optimise this pretty well.

>     friend bool operator != (const ptr& p1, const ptr& p2)
>     {
>        return !(p1 == p2);
>     }

No need to make this one a friend, it doesn't use any private parts. Note
that even operator== could have been implemented with just the public
interface.

One final note: boost::intrusive_ptr does pretty much all that your pointer
does and a bit more. If you intend yours for production code, I'd at least
take a look at that one for inspiration if not completely switch to it.

Uli


--
      [ See http://www.gotw.ca/resources/clcm.htm for info about ]
      [ comp.lang.c++.moderated.    First time posters: Do this! ]

On Sep 29, 11:01 pm, Davin Pearson <davin.pear...@gmail.com> wrote:
> I have written a smart pointer class called ptr<X>
> Here are two ways it could be used:
>
> void foo(ptr<X> p)
> {
>      // use p here
>
> }
>
> void foo(const ptr<X>& p)
> {
>     ASSERT(&p != null);
>     // use p here
>
> }
>
> Based on my work it seems that boths ways are equally efficient
> because the size of the ptr<X> class is 4 bytes (enough to hold a
> pointer variable) and therefore the variable can be passed inside a
> register.  Therefore it makes sense to use the first way because it is
> simpler.

Did you consider that the first way would cause the ref_counter to be
incremented? And then decremented on function return? This probably is
more expensive than the reference

Lance





--
      [ See http://www.gotw.ca/resources/clcm.htm for info about ]
      [ comp.lang.c++.moderated.    First time posters: Do this! ]

On Oct 1, 5:52 am, Ulrich Eckhardt <dooms...@knuut.de> wrote:
> Davin Pearson wrote:
> > I have written a smart pointer class called ptr<X>
> > Here are two ways it could be used:
>
> > void foo(ptr<X> p)
> > {
> >      // use p here
> > }
>
> > void foo(const ptr<X>& p)
> > {
> >     ASSERT(&p != null);
> >     // use p here
> > }
>
> Um, why that assertion? If you have a reference, and taking the address of
> that reference yields NULL, you have undefined behaviour (NULL pointer
> dereference) already anyway!
>

Here is some code that explains why you need to
take the address of the reference:

#include <...>

class X
{
   int ref_count;
};

void foo(const ptr<X>& p)
{
   assert(&p != null);
}

int main()
{
   ptr<X>* p = null;
   foo(*p);
   exit(EXIT_SUCCESS);
}

The assertion fails because you are passing in null as a reference.

After some empirical testing I have found out that
foo(const ptr<X>&) is faster than foo(ptr<X>).
Therefore I will use the first alternative
in my code.


-- 
      [ See http://www.gotw.ca/resources/clcm.htm for info about ]
      [ comp.lang.c++.moderated.    First time posters: Do this! ]

Hi

Davin Pearson wrote:

> Here is some code that explains why you need to
> take the address of the reference:
[...]
> int main()
> {
>    ptr<X>* p = null;

I assume ptr<X>* p = 0? Or a preceding #define null 0?

>    foo(*p);
>    exit(EXIT_SUCCESS);
> }
> 
> The assertion fails because you are passing in null as a reference.

Well... as Ulrich noted, this is undefined behavior, whether you include an
assertion or you don't. (The "*p" makes it undefined, because you are
dereferencing a null pointer.) The program might crash before the
assertion, it might not crash at all, your assertion might print "Happy New
Year!"... the possibilities are endless.

It makes no sense to check whether undefined behavior has been invoked,
because at that point there is no guarantee that the check itself will do
anything sensible.

Markus


-- 
      [ See http://www.gotw.ca/resources/clcm.htm for info about ]
      [ comp.lang.c++.moderated.    First time posters: Do this! ]

Markus Moll wrote:
[ about  'assert( this!=0);' and similar things ]
> It makes no sense to check whether undefined behavior has been invoked,
> because at that point there is no guarantee that the check itself will do
> anything sensible.

Sorry, but I beg to differ. The whole 'checked STL' idea is about giving a
proper diagnostic for things that the C++ standard only calls 'undefined'.
However, and that's my reason for the initial comment on that, a null
pointer dereference is by far not as hard to find as an iterator overflow,
so it doesn't merit an assertion. Whether you have an assertion that fails
and dumps you into the debugger or whether it's access to the object which
does it doesn't matter, hence those assertions aren't worth the effort for
me and just clutter up the code.

BTW: I don't mean that checking should be performed, but two different
versions, one with speed the other with diagnostics are a good idea.

Uli


-- 
      [ See http://www.gotw.ca/resources/clcm.htm for info about ]
      [ comp.lang.c++.moderated.    First time posters: Do this! ]

On Oct 1, 3:13 pm, Davin Pearson <davin.pear...@gmail.com> wrote:
> On Oct 1, 5:52 am, Ulrich Eckhardt <dooms...@knuut.de> wrote:
> > Davin Pearson wrote:
> > > void foo(const ptr<X>& p)
> > > {
> > >     ASSERT(&p != null);
> > >     // use p here
> > > }
>
> > Um, why that assertion? If you have a reference, and taking the address of
> > that reference yields NULL, you have undefined behaviour (NULL pointer
> > dereference) already anyway!
>
> Here is some code that explains why you need to
> take the address of the reference:
>
> ...
> int main()
> {
>    ptr<X>* p = null;
>    foo(*p);
>    exit(EXIT_SUCCESS);
> }
>
> The assertion fails because you are passing in null as a reference.

As Markus Moll pointed out the standard says that this
has undefined behaviour. On the other hand, if the code
is only intended for a specific platform and compiler,
assertions that this!=NULL may sometimes be useful. Then
again, why should you do it in a smart pointer class?
How often are you going to use a pointer-to-pointer?

Note that the following will not trigger the assertion
in copy constructor on any platform and compiler I know:

struct X{
    int a;
    ptr<int> p;
};

int main(){
   X* pX = NULL;
   ptr<int> test(pX->p); //< dereferencing NULL here
}

> After some empirical testing I have found out that
> foo(const ptr<X>&) is faster than foo(ptr<X>).
> Therefore I will use the first alternative
> in my code.

Well, when taking the argument by reference, you avoid
checking for NULL and incrementing/decrementing the
ref_count. So this is the expected result. (But you
never know what the compiler can optimize or fail to
optimize just because of the little change. It all
depends on the context.)

Cheers,
Vladimir Marko


-- 
      [ See http://www.gotw.ca/resources/clcm.htm for info about ]
      [ comp.lang.c++.moderated.    First time posters: Do this! ]

On Oct 2, 7:58 am, Markus Moll <markus.m...@esat.kuleuven.ac.be>
wrote:
> Davin Pearson wrote:
> > Here is some code that explains why you need to
> > take the address of the reference:
> [...]
> > int main()
> > {
> >    ptr<X>* p = null;
>
> I assume ptr<X>* p = 0? Or a preceding #define null 0?

Yes that's correct.

>
> >    foo(*p);
> >    exit(EXIT_SUCCESS);
> > }
>
> > The assertion fails because you are passing in null as a reference.
>
> Well... as Ulrich noted, this is undefined behavior, whether you include an
> assertion or you don't. (The "*p" makes it undefined, because you are
> dereferencing a null pointer.) The program might crash before the
> assertion, it might not crash at all, your assertion might print "Happy New
> Year!"... the possibilities are endless.
>

For my purposes it is better to get an assertion failure than a crash
because you get
more information with an assertion failure than a crash.

The *p works fine on my machine.  The program would crash if you try
to use
p like so:

ptr<X> q = p;

By putting the assertion before you use p causes you to get an
assertion failure rather than a program crash.


-- 
      [ See http://www.gotw.ca/resources/clcm.htm for info about ]
      [ comp.lang.c++.moderated.    First time posters: Do this! ]

Hi

Ulrich Eckhardt wrote:

> Sorry, but I beg to differ. The whole 'checked STL' idea is about giving a
> proper diagnostic for things that the C++ standard only calls 'undefined'.

Yes, but that only works if your implementation gives you some guarantees
about that. And then it is not portable code any more. Don't misunderstand
me: If that's fine for you, it's fine for me. But in general, such
assertions are completely pointless.

Markus


-- 
      [ See http://www.gotw.ca/resources/clcm.htm for info about ]
      [ comp.lang.c++.moderated.    First time posters: Do this! ]

Hi

Davin Pearson wrote:

> For my purposes it is better to get an assertion failure than a crash
> because you get
> more information with an assertion failure than a crash.

(Hint: debugger)

> The *p works fine on my machine.  The program would crash if you try
> to use
> p like so:
> 
> ptr<X> q = p;
> 
> By putting the assertion before you use p causes you to get an
> assertion failure rather than a program crash.

Well, maybe on your machine and until now. But unless you can find something
about that in your compiler's documentation, there is always a good chance
that with some border cases, it will not trigger the assertion but instead
crash (or do whatever else). IMO, assertions like these give you a false
feeling of safety.

Markus


-- 
      [ See http://www.gotw.ca/resources/clcm.htm for info about ]
      [ comp.lang.c++.moderated.    First time posters: Do this! ]

Markus Moll wrote:
> Ulrich Eckhardt wrote:
>> Sorry, but I beg to differ. The whole 'checked STL' idea is about giving
>> a proper diagnostic for things that the C++ standard only calls
>> 'undefined'.
> 
> Yes, but that only works if your implementation gives you some guarantees
> about that. And then it is not portable code any more. Don't misunderstand
> me: If that's fine for you, it's fine for me. But in general, such
> assertions are completely pointless.

You mean that

  class vector {
    T const& operator[](size_type i) const {
      assert(i<size);
      return *(begin()+i);
    }
    ...
  };

is useless or not portable? Note that in this case, no undefined behaviour
occurs when this function is called with an invalid index and NDEBUG is not
defined. In the case that the OP used, that UB has already occurred when it
is detected via some implementation-specific means.

Uli

-- 
Sator Laser GmbH
Geschäftsführer: Ronald Boers, Amtsgericht Hamburg HR B62 932


      [ See http://www.gotw.ca/resources/clcm.htm for info about ]
      [ comp.lang.c++.moderated.    First time posters: Do this! ]

Hi

Ulrich Eckhardt wrote:

> Markus Moll wrote:
>> Yes, but that only works if your implementation gives you some guarantees
>> about that. And then it is not portable code any more. Don't
>> misunderstand me: If that's fine for you, it's fine for me. But in
>> general, such assertions are completely pointless.
> 
> You mean that
> 
>   class vector {
>     T const& operator[](size_type i) const {
>       assert(i<size);
>       return *(begin()+i);
>     }
>     ...
>   };
> 
> is useless or not portable?

There seems to be a misunderstanding. No, of course I _don't_ think that's
useless, nor do I think it is not portable. What I _said_ was that I think
that

>> It makes no sense to check whether undefined behavior has been invoked

This is quite different, because it means checking for possible undefined
behavior when in fact the behavior already is undefined (including the
behavior of any such check). It's as pointless as constantly checking
whether you are still alive or already dead. There is no guarantee that you
will ever notice that you died... It might be different if God spoke to you
before and gave you those guarantees. ;-) (Even then it's not necessarily
portable across beliefs... euh... okay, I'll stop here)

> Note that in this case, no undefined behaviour 
> occurs when this function is called with an invalid index and NDEBUG is
> not defined. In the case that the OP used, that UB has already occurred
> when it is detected via some implementation-specific means.

Well... the question remains whether it really is an implementation-specific
means, i.e. whether it is detected. And "relying on implementation-specific
guarantees" and "portable" mean different things to me.

Markus


-- 
      [ See http://www.gotw.ca/resources/clcm.htm for info about ]
      [ comp.lang.c++.moderated.    First time posters: Do this! ]