Feedback on my C++ program

[DumbestPerson224]

Body

template <typename T> 
class auto_ptr{
private:
    T* pointer;
    
public:
    // This boolean holds whether or not the pointer is going to be automatically managed
    bool auto_manage = true;
    
    // Setup class construction and deconstruction
    
    auto_ptr(){
        if(auto_manage == true){
            pointer = new T;
        }
    }
    
    ~auto_ptr(){
        if(auto_manage == true){
            delete pointer;
        }
    }
    
    void alllocate(){
        if(auto_manage == false){
            pointer = new T;
        }
    }
    
    void deallocate(){
        if(auto_manage == false){
            delete pointer;
        }
    }
    
    /**
     This allocates ptr and makes the pointer `pointer` point to ptr 
     * @brief Setup the pointer
     * @param ptr
    **/
    auto_ptr(T* ptr) : pointer (ptr){};
    
    /**
        * @brief Handle operator overloading
        * @returns The memory address of pointer 
    **/
    T* operator -> (){
        return pointer;
    }
};

Guidelines

• I have read and understood this category's guidelines before making this post.

[natasabrunet]

Yes. This auto_ptr class is a bit outdated and can lead to memory management issues, Using smart pointers or std::shared_ptr would be a better way.

[DumbestPerson224]

I didn't think about that. That is a good idea. It would be a lot safer.

[natasabrunet]

Great!

[JerryHung1030]

Hi @DumbestPerson224,
Your auto_ptr implementation is an interesting exercise in creating a custom smart pointer class! However, I would like to point out a few issues and improvements for better functionality, safety, and alignment with modern C++ practices:

Issues and Suggestions:

1. Potential Memory Leaks:

   • The constructor auto_ptr() doesn't initialize pointer unless auto_manage is true. If auto_manage is false, pointer remains uninitialized, which can lead to undefined behavior when deallocate() or the destructor is called.
   • Suggestion: Initialize pointer to nullptr in all constructors to ensure safety.

2. Incomplete Ownership Semantics:

   • Your class lacks copy and assignment operators. Copying or assigning an auto_ptr instance would result in shallow copying of the pointer, leading to double deletions.
   • Suggestion: Implement proper copy/move constructors and copy/move assignment operators (or delete them explicitly if ownership should be unique).

3. Redundant Memory Management:

   • You manage memory manually with allocate() and deallocate(), which defeats the purpose of smart pointers. A smart pointer should abstract this away.

4. Alignment with Modern C++:

   • The use of auto_ptr is deprecated in modern C++ (removed in C++17). Consider using std::unique_ptr or std::shared_ptr instead, as they are safer and more feature-complete.

Improved Version:

If you'd like to keep practicing and refining your implementation, here's an improved version based on your current approach:

#include <iostream>

template <typename T>
class auto_ptr {
private:
    T* pointer = nullptr;

public:
    bool auto_manage = true;

    // Default constructor
    auto_ptr() {
        if (auto_manage) {
            pointer = new T();
        }
    }

    // Constructor accepting a raw pointer
    explicit auto_ptr(T* ptr) : pointer(ptr) {}

    // Destructor
    ~auto_ptr() {
        if (auto_manage && pointer) {
            delete pointer;
        }
    }

    // Prevent copy semantics
    auto_ptr(const auto_ptr&) = delete;
    auto_ptr& operator=(const auto_ptr&) = delete;

    // Enable move semantics
    auto_ptr(auto_ptr&& other) noexcept : pointer(other.pointer), auto_manage(other.auto_manage) {
        other.pointer = nullptr;
    }
    auto_ptr& operator=(auto_ptr&& other) noexcept {
        if (this != &other) {
            if (auto_manage && pointer) {
                delete pointer;
            }
            pointer = other.pointer;
            auto_manage = other.auto_manage;
            other.pointer = nullptr;
        }
        return *this;
    }

    // Access the managed object
    T* operator->() const {
        return pointer;
    }

    T& operator*() const {
        return *pointer;
    }
};

Explanation of Changes:

1. Initialization: Ensured pointer is always initialized to nullptr to avoid undefined behavior.
2. Copy/Move Semantics: Deleted copy operations and added move semantics for better ownership management.
3. Modern C++ Style: Used explicit for single-argument constructors to prevent implicit conversions.

Keep up the good work and happy coding! 😊

[DumbestPerson224]

I recently switched to shared pointers for memory safety.

[JerryHung1030]

That's a great decision,👍 Switching to std::shared_ptr (or even std::unique_ptr for unique ownership) ensures much better memory safety and reduces the risk of issues like double deletion or memory leaks.

[DumbestPerson224]

I am having trouble accessing the class I did this:

    automatic_pointer<int> pointer = automatic_ptr<int>(std::make_shared<int>(new int (0)));