Probably another bad idea... Pointer question..

Here's the code. The question follows.

Code:

---

#pragma once
#define SAFE_DELETE(var) if ( var ) { delete var; var = 0; }
template<class T>
class Type
{
public:
    //Default constructor
    Type()
        :is_good(false),
        typePtr(0)
    {/*Empty code block*/};

    //Assignment constructor
    Type(const T& val)
        :is_good(false),
        typePtr(0)
    {
        //Allocate memory with assigned value
        typePtr = new T(val);
        if ( *typePtr == val )
            is_good = bool(true);
        else
        {
            SAFE_DELETE(typePtr);
        }
    };

    //Copy constructor
    //Does not copy pointers. Copies values.
    Type(const Type& copy)
        :is_good(false),
        typePtr(0)
    {
        //Allocate memory with copy values
        typePtr = new T(*copy.typePtr);
        if ( *typePtr == *copy.typePtr )
            is_good = bool(true);
        else
        {
            SAFE_DELETE(typePtr);
        }
    };

    //Destructor
    ~Type()
    {
        is_good = false;
        SAFE_DELETE(typePtr);
    };

    //Copy assignment
    const Type& operator=(const Type& copy)
    {
        if ( typePtr )
        {
            is_good = bool(false);
            SAFE_DELETE(typePtr);
        }
        typePtr = new T(*copy.typePtr);
        is_good = copy.is_good;

        return *this;
    };

    T& operator*()
    {
        return *typePtr;
    };

    T* operator->()
    {
        return typePtr;
    };

    operator bool()
    {
        return is_good;
    };
private:
    bool is_good;

    //The pointer to the type
    T* typePtr;
};

#undef SAFE_DELETE

---

Ok, first off. The SAFE_DELETE is defined then undefined, because most of my classes need a different version of it.

The point of the class is to wrap the handling of the pointer. Pretty much to disallow modifying the pointer without ugly code.
My thought on this is that with this class I can do something like the following.

Code:

---

Type<int> Test(100);
if ( Test )//check if the ptr is good
  std::cout << *Test << std::endl;

//As opposed to
int * Test = new int(100);
if ( Test != 0 )
  std::cout << *Test << std::endl;
delete Test;

---

I realize that there isn't much difference between them, but I thought I would see what you guys thought. Well Thank you.

EDIT:
Also, if I am not mistaken the only way to directly modify the pointer, or to prematurely delete would be to do this right?

Code:

---

Type<int> Test(100);
delete Test.operator->();//Am i right?

---

I only saw one question, and the answer to that question: Ewww! Anyone writing that deserves to have it blow up; don't worry about it.

Soma

So is the class a good/mediocre/bad idea?

Code:

---

define SAFE_DELETE(var) if ( var ) { delete var; var = 0; }

---

this isn't really necessary since deleting a NULL pointer is guaranteed to be safe.

>> Also, if I am not mistaken the only way to directly modify the pointer, or to prematurely delete would be to do this right?

well, the user could simple obtain the reference to the data via operator *, then pass it's address to operator delete.

Quote:

---

So is the class a good/mediocre/bad idea?

---

Ah... it isn't a bad idea per say... just very incomplete. (For example, if you give it a polymorphic type your copy mechanism will fail. If you change the behavior of the copy mechanism indirectly through a customizable--template class that can be specialized, interface you could allow a provider of such a polymorphic class to support your pointer-thing. You should still provide a default behavior matching what you have for all normal types.) I see only three paths by which this might evolve: 1) It evolves until it mimics 'copy_ptr'. 2) It evolves until it mimics 'std::vector<?>'. 3) It evolves until it mimics Alexandrescu's policy based pointers--providing a slightly odd mechanic for at least one policy.

So... do you want to reinvent the wheel? If so, go for it. (Unlike most people, I think reinventing the wheel is good for you; you are bound to learn something if you are unfamiliar with that particular flavor of wheel.)

Soma

Instead of that SAFE_DELETE macro, use a safer suggestion by Stroustrup:

Code:

---

template<typename T>
inline void destroy(T*& p)
{
    delete p;
    p = 0;
}

---

Quote:

---

The SAFE_DELETE is defined then undefined, because most of my classes need a different version of it.

---

That's a very bad idea. If you have to have a macro, at least make it do the same thing everywhere. Macros are confusing enough as they are, without making them context-specific.

Code:

---

        typePtr = new T(val);
        if ( *typePtr == val )
            is_good = bool(true);

---

This piece of code is bad. It introduces the requirement that all types you allocate provide ==. It does this for the highly suspicious reason of checking the value of the allocated object for correctness. This doesn't make sense. The value will be correct in 100&#37; of the cases, unless you have a buggy copy constructor. The reaction to a buggy copy constructor is not to delete the object and move on. It's to let the program crash and burn and scream loudly for its mummy (you) to fix it.
And the smart pointer is the wrong place to find out about the buggy cc.
As a minor WTF in the code, you take the boolean constant true and explicitly convert it to bool. What for?
The whole is_good variable is nonsense, and the bool conversion operator should return whether the pointer is null or not.

Code:

---

    //The pointer to the type
    T* typePtr;

---

This is not so much a functional but a conceptional problem. The pointer points to an object, not a type. Thus, the comment and the variable name are wrong and misleading.

When writing smart pointers, there are four well-known copy strategies: disallow (boost::scoped_ptr), move (std::auto_ptr, C++0x std::unique_ptr), share (boost::shared_ptr) and clone (various clone_ptr implementations, and your class). Of these, clone is the least useful. This is for a reason.
Let's review when you use dynamic allocation.
1) When you want a local object that's simply too big for the stack.
The stack is quite limited - to 1MB for standard Win32 linkage. If you want to allocate a big buffer of 1 MB, you can't just place an array on the stack. You have to allocate it.
Because the variable is local, all you need is a single smart pointer. You never want to copy the pointer. Thus, disallow is the right strategy for this scenario.
2) When you want an object to survive the end of the function.
Usually, you'll probably add the object to a container and not use smart pointers at all. But there are exceptions.
For example, adding the object to a container creates a copy. Copying the object may be impossible or undesirable, though, so you make a container of pointers. What pointer will you choose for the container? Disallow is out - it's not copyable and thus not suitable for a container. Clone is out, too - the point of the exercise is to avoid copying the object.
You'll probably use share or, in C++0x, move. Those are safe to put in a container, and they avoid the copy.
You might want to simply return the object from the function. Again, disallow and clone are out - one disallows the necessary copying, and if you use clone, you might as well return the object directly. In this scenario, you'll use move.
3) You want to pass something around that can't be copied.
Obviously, disallow and clone are not applicable. Again! You'll probably use share for this, though move might be usable.

As you can see, it's very hard to find a real use case for clone pointers. The other three are generally more useful.



Your copy assignment operator doesn't provide the strong exception safety guarantee, even though such a thing is very easy to achieve. Copy-swap is just one way. Speaking of swap, you should definitely provide a swap member function.

Quote:

---

Originally Posted by CornedBee

When writing smart pointers, there are four well-known copy strategies: disallow (boost::scoped_ptr), move (std::auto_ptr, C++0x std::unique_ptr), share (boost::shared_ptr) and clone (various clone_ptr implementations, and your class). Of these, clone is the least useful. This is for a reason.

---

A clone_ptr would relieve you of having to write deep copy constructors, if the class holds its internal pointers in clone_ptrs. I'd call that a huge win.

Do you have many classes that have pointer to single objects they own as members? I don't.

A clone_array might be quite useful. It has to store the length, obviously.

Quote:

---

Originally Posted by CornedBee

Do you have many classes that have pointer to single objects they own as members? I don't.

---

Not really. I've only needed such a thing a few times, but it was nice to not be forced to write a huge redundant copy constructor, which I can potentially make a typo in, just to deal with a single weird field.

Since value semantics rule, a clone_ptr would be important for use with classes and class hierarchies that you wish to have keep those value semantics, right?

I've run into a couple of occasions where I would like to have used a clone_ptr without the extra headache of having to write a clone() function. I think using a pimpl with a class you wish to be copyable was one example. That said, I've never actually gone ahead and done it, so I can't say whether it would truly be useful or not.

Yes, I never said there wasn't a use case, only that it was hard to find one. (OK, so not that hard. But there aren't many.)

Of course, that leaves the problem of dealing with base pointers.