Skip to main content
replaced http://codereview.stackexchange.com/ with https://codereview.stackexchange.com/
Source Link

Contrived bool conversion:

@AlchemicalApplesAlchemicalApples beat me to it, but I'm going to say it anyways.

Your implementation looks quite good, however, there is the issue of operator bool and the implicit conversions it allows.

I've dug up my copy of Modern C++ Design and re-read the chapter on smart pointers. It goes into a lot of detail about the issues related to providing safe and as idiomatic as possible comparison operators. I suggest that you read it yourself.

The problem with overloading operator bool is that code such at the following will compile:

int main()
{
    Loki::UniquePtr<int>   data1(new int(5));
    Loki::UniquePtr<float> data2(new float(6.0));

    if (data1 == data2) // Will be true!
    {
        std::cout << "Problem!\n";
    }

    bool b = data1; // Unusual, but...

    if ((data1 * 5) == 200) // Starts to behave like an integer, doesn't it?
    {
    }
}

It is still useful, however, to be able to test a pointer in the traditional if (ptr) ... way. To achieve that syntax, the best way is to explicitly provide each comparison operator (==, !=, etc) and then use the "tester type" trick presented in the book, which boils down to replacing the bool operator with a conversion to a dummy pointer type that can't be deleted:

class UniquePtr
{
    // Don't let it get deleted outside.
    class Tester 
    {
        void operator delete (void *);
    };

public:

    // Allows safe and correct "if (ptr)..." checks and is an actual pointer type.
    operator Tester*() const
    {
        if (! data)
        {
            return nullptr;
        }
        static Tester tester;
        return &tester;
    }
};

The example above is adapted directly from the book, so again, I recommend reading it to get a much more complete explanation of why this works.

new mixed with delete[]:

Another thing out of place that I've spotted is that your destructor is deleting an array, while in your example, it allocated a single element:

~UniquePtr()
{
    delete [] data;
}

This might work quietly when dealing with native types that have no destructors, but will fail catastrophically for types that have destructors. Nevertheless, it is a mistake and should be fixed.

Contrived bool conversion:

@AlchemicalApples beat me to it, but I'm going to say it anyways.

Your implementation looks quite good, however, there is the issue of operator bool and the implicit conversions it allows.

I've dug up my copy of Modern C++ Design and re-read the chapter on smart pointers. It goes into a lot of detail about the issues related to providing safe and as idiomatic as possible comparison operators. I suggest that you read it yourself.

The problem with overloading operator bool is that code such at the following will compile:

int main()
{
    Loki::UniquePtr<int>   data1(new int(5));
    Loki::UniquePtr<float> data2(new float(6.0));

    if (data1 == data2) // Will be true!
    {
        std::cout << "Problem!\n";
    }

    bool b = data1; // Unusual, but...

    if ((data1 * 5) == 200) // Starts to behave like an integer, doesn't it?
    {
    }
}

It is still useful, however, to be able to test a pointer in the traditional if (ptr) ... way. To achieve that syntax, the best way is to explicitly provide each comparison operator (==, !=, etc) and then use the "tester type" trick presented in the book, which boils down to replacing the bool operator with a conversion to a dummy pointer type that can't be deleted:

class UniquePtr
{
    // Don't let it get deleted outside.
    class Tester 
    {
        void operator delete (void *);
    };

public:

    // Allows safe and correct "if (ptr)..." checks and is an actual pointer type.
    operator Tester*() const
    {
        if (! data)
        {
            return nullptr;
        }
        static Tester tester;
        return &tester;
    }
};

The example above is adapted directly from the book, so again, I recommend reading it to get a much more complete explanation of why this works.

new mixed with delete[]:

Another thing out of place that I've spotted is that your destructor is deleting an array, while in your example, it allocated a single element:

~UniquePtr()
{
    delete [] data;
}

This might work quietly when dealing with native types that have no destructors, but will fail catastrophically for types that have destructors. Nevertheless, it is a mistake and should be fixed.

Contrived bool conversion:

@AlchemicalApples beat me to it, but I'm going to say it anyways.

Your implementation looks quite good, however, there is the issue of operator bool and the implicit conversions it allows.

I've dug up my copy of Modern C++ Design and re-read the chapter on smart pointers. It goes into a lot of detail about the issues related to providing safe and as idiomatic as possible comparison operators. I suggest that you read it yourself.

The problem with overloading operator bool is that code such at the following will compile:

int main()
{
    Loki::UniquePtr<int>   data1(new int(5));
    Loki::UniquePtr<float> data2(new float(6.0));

    if (data1 == data2) // Will be true!
    {
        std::cout << "Problem!\n";
    }

    bool b = data1; // Unusual, but...

    if ((data1 * 5) == 200) // Starts to behave like an integer, doesn't it?
    {
    }
}

It is still useful, however, to be able to test a pointer in the traditional if (ptr) ... way. To achieve that syntax, the best way is to explicitly provide each comparison operator (==, !=, etc) and then use the "tester type" trick presented in the book, which boils down to replacing the bool operator with a conversion to a dummy pointer type that can't be deleted:

class UniquePtr
{
    // Don't let it get deleted outside.
    class Tester 
    {
        void operator delete (void *);
    };

public:

    // Allows safe and correct "if (ptr)..." checks and is an actual pointer type.
    operator Tester*() const
    {
        if (! data)
        {
            return nullptr;
        }
        static Tester tester;
        return &tester;
    }
};

The example above is adapted directly from the book, so again, I recommend reading it to get a much more complete explanation of why this works.

new mixed with delete[]:

Another thing out of place that I've spotted is that your destructor is deleting an array, while in your example, it allocated a single element:

~UniquePtr()
{
    delete [] data;
}

This might work quietly when dealing with native types that have no destructors, but will fail catastrophically for types that have destructors. Nevertheless, it is a mistake and should be fixed.

Minor grammar fixes.
Source Link
glampert
  • 17.3k
  • 4
  • 31
  • 89
int main()
{
    Loki::UniquePtr<int>   data1(new int(5));
    Loki::UniquePtr<float> data2(new float(6.0));

    if (data1 == data2) // Will be true!
    {
        std::cout << "Problem!\n";
    }

    bool b = data1; // Unusual, but...

    if ((data1 * 5) == 200) // StartStarts to behave like an integer, doesn't it?
    {
    }
}

It is still useful, however, to be able to test a pointer in the traditional if (ptr) ... way. To achieve that syntax, the best way is to explicitly provide each comparison operator (==, !=, etc) and then use the "bool tester""tester type" trick presented in the book, which boils down to replacing the bool operator with a conversion to a dummy pointer type that can't be deleted:

class UniquePtr
{
    // Don't let it get deleted outside.
    class Tester 
    {
        void operator delete (void *);
    };

public:

    // Allows safe and correct "if (ptr)..." checks and is an actual pointer type.
    operator Tester*() const
    {
        if (! data)
        {
            return nullptr;
        }
        static Tester tester;
        return &tester;
    }
};

Another thing out of place that I've noticedspotted is that your destructor is deleting an array, while in your example, it allocated a single element:

This shouldmight work finequietly when dealing with native types that have no destructors, but will probably fail catastrophically for types that have destructors. Nevertheless, it is a mistake and should be fixed.

int main()
{
    Loki::UniquePtr<int>   data1(new int(5));
    Loki::UniquePtr<float> data2(new float(6.0));

    if (data1 == data2) // Will be true!
    {
        std::cout << "Problem!\n";
    }

    bool b = data1; // Unusual, but...

    if ((data1 * 5) == 200) // Start to behave like an integer, doesn't it?
    {
    }
}

It is still useful, however, to be able to test a pointer in the traditional if (ptr) ... way. To achieve that syntax, the best way is to explicitly provide each comparison operator (==, !=, etc) and then use the "bool tester" trick presented in the book, which boils down to replacing the bool operator with a conversion to a dummy pointer type that can't be deleted:

class UniquePtr
{
    // Don't let it get deleted outside.
    class Tester 
    {
        void operator delete (void *);
    };

public:

    // Allows safe and correct "if (ptr)..." checks.
    operator Tester*() const
    {
        if (! data)
        {
            return nullptr;
        }
        static Tester tester;
        return &tester;
    }
};

Another thing out of place that I've noticed is that your destructor is deleting an array, while in your example, it allocated a single element:

This should work fine when dealing with types that have no destructors, but will probably fail catastrophically for types that have destructors.

int main()
{
    Loki::UniquePtr<int>   data1(new int(5));
    Loki::UniquePtr<float> data2(new float(6.0));

    if (data1 == data2) // Will be true!
    {
        std::cout << "Problem!\n";
    }

    bool b = data1; // Unusual, but...

    if ((data1 * 5) == 200) // Starts to behave like an integer, doesn't it?
    {
    }
}

It is still useful, however, to be able to test a pointer in the traditional if (ptr) ... way. To achieve that syntax, the best way is to explicitly provide each comparison operator (==, !=, etc) and then use the "tester type" trick presented in the book, which boils down to replacing the bool operator with a conversion to a dummy pointer type that can't be deleted:

class UniquePtr
{
    // Don't let it get deleted outside.
    class Tester 
    {
        void operator delete (void *);
    };

public:

    // Allows safe and correct "if (ptr)..." checks and is an actual pointer type.
    operator Tester*() const
    {
        if (! data)
        {
            return nullptr;
        }
        static Tester tester;
        return &tester;
    }
};

Another thing out of place that I've spotted is that your destructor is deleting an array, while in your example, it allocated a single element:

This might work quietly when dealing with native types that have no destructors, but will fail catastrophically for types that have destructors. Nevertheless, it is a mistake and should be fixed.

Source Link
glampert
  • 17.3k
  • 4
  • 31
  • 89

Contrived bool conversion:

@AlchemicalApples beat me to it, but I'm going to say it anyways.

Your implementation looks quite good, however, there is the issue of operator bool and the implicit conversions it allows.

I've dug up my copy of Modern C++ Design and re-read the chapter on smart pointers. It goes into a lot of detail about the issues related to providing safe and as idiomatic as possible comparison operators. I suggest that you read it yourself.

The problem with overloading operator bool is that code such at the following will compile:

int main()
{
    Loki::UniquePtr<int>   data1(new int(5));
    Loki::UniquePtr<float> data2(new float(6.0));

    if (data1 == data2) // Will be true!
    {
        std::cout << "Problem!\n";
    }

    bool b = data1; // Unusual, but...

    if ((data1 * 5) == 200) // Start to behave like an integer, doesn't it?
    {
    }
}

It is still useful, however, to be able to test a pointer in the traditional if (ptr) ... way. To achieve that syntax, the best way is to explicitly provide each comparison operator (==, !=, etc) and then use the "bool tester" trick presented in the book, which boils down to replacing the bool operator with a conversion to a dummy pointer type that can't be deleted:

class UniquePtr
{
    // Don't let it get deleted outside.
    class Tester 
    {
        void operator delete (void *);
    };

public:

    // Allows safe and correct "if (ptr)..." checks.
    operator Tester*() const
    {
        if (! data)
        {
            return nullptr;
        }
        static Tester tester;
        return &tester;
    }
};

The example above is adapted directly from the book, so again, I recommend reading it to get a much more complete explanation of why this works.

new mixed with delete[]:

Another thing out of place that I've noticed is that your destructor is deleting an array, while in your example, it allocated a single element:

~UniquePtr()
{
    delete [] data;
}

This should work fine when dealing with types that have no destructors, but will probably fail catastrophically for types that have destructors.