Dtyjlui

Edit: NOTE I'm a C++ "beginner" still in undergrad trying to teach myself modern C++ (because they don't do that in uni) so I'm sure this is riddled with errors that I am unaware of.

Made a subset of std's smart pointer library (unique, shared, and weak, denoted as unq, shr, and weak) in a very minimal header file. This is mostly for fun and as a learning experience but looking to improve in any way, thanks!

ptr.h

#ifndef PTRLIB_H                                                                                                        

#define PTRLIB_H                                                                                                                                                                                                                                



#include <cstdint>                                                                                                      

#include <atomic>                                                                                                       

#include <iostream>                                                                                                                                                                                                                             



namespace ptr                                                                                                           

{                                                                                                                               

    template<typename T>                                                                                                    

    class base //for methods common to all smart ptr types                                                                  

    {                                                                                                                                               

        protected:                                                                                                                      

            mutable T * obj;                                                                                                                                                                                                                                



            //non instatiable outside the header                                                                                     

            base() {}                                                                                                               

            base(T * obj) : obj(obj) {}                                                                                                                                                                                                                     



            virtual void operator = (T * obj) { this->obj = obj; }                                                                                                                                                                                  



        public:                                                                                                                         

            //unq uses these versions                                                                                               

            virtual void reset() { delete this->obj; this->obj = nullptr; }                                                         

            virtual void reset(T * obj) { delete this->obj; this->obj = obj; }                                                                                                                                                                              



            inline T * get() { return obj; }                                                                                                                                                                                                                



            operator bool const () { return (obj != nullptr) ? true : false; }                                                                                                                                                                              



            bool operator == (const base<T> rhs) { return obj == rhs.obj; }                                                         

            bool operator != (const base<T> rhs) { return obj != rhs.obj; }                                                         

            bool operator <= (const base<T> rhs) { return obj <= rhs.obj; }                                                         

            bool operator >= (const base<T> rhs) { return obj >= rhs.obj; }                                                         

            bool operator < (const base<T> rhs) { return obj < rhs.obj; }                                                           

            bool operator > (const base<T> rhs) { return obj > rhs.obj; }                                                                                                                                                                                   



            std::ostream & operator << (std::ostream & stream) { return (std::cout << obj); }                                                                                                                                           

    };                                                                                                                                                                                                                                              



    template<typename T>                                                                                                    

    class unq : public base<T>                                                                                              

    {                                                                                                                               

        public:                                                                                                                         

            unq() {}                                                                                                                

            unq(T * obj) : base<T>(obj) {}                                                                                          

            unq(const unq<T> & u) : base<T>(u.obj) { u.obj = nullptr; }

            ~unq() { delete this->obj; }                                                                                                                                                                                                                    



            T * release()                                                                                                           

            {                                                                                                                               

                T * temp = this->obj;                                                                                                   

                this->obj = nullptr;                                                                                                    

                return temp;                                                                                                    

            }                                                                                                                                                                                                                                               



            //don't want weak to be able to access the object so duplicated in shr                                                  

            inline T * operator -> () { return this->obj; }                                                                         

            inline T & operator * () { return *(this->obj); }                                                       

    };                                                                                                                                                                                                                                              



    template<typename T>                                                                                                    

    class weak; //class forwarding for friend class                                                                                                                                                                                                 



    template<typename T>                                                                                                    

    class shr : public base<T>                                                                                              

    {                                                                                                                               

        private:                                                                                                                        

            friend class weak<T>;                                                                                                                                                                                                                           



            //reference counter                                                                                                     

            mutable std::atomic<int32_t> * refs;                                                                                                                                                                                                            



            inline bool is_last() { return ((refs == nullptr && this->obj == nullptr) || *refs == 1); }                                                                                                                                             



        public:                                                                                                                         

            shr()                                                                                                                   

            { refs = new std::atomic<int32_t>, *refs = 1; }                                                                                                                                                                                                 



            shr(T * obj) : base<T>(obj)                                                                                             

            { refs = new std::atomic<int32_t>, *refs = 1; }                                                                                                                                                                                                 



            shr(const shr<T> & s) : base<T>(s.obj)                                                                                  

            { refs = (s.refs != nullptr) ? s.refs : new std::atomic<int32_t>, *refs += 1; }                                                                                                                                                                 



            shr(const weak<T> & w) : base<T>(w.obj)                                                                                 

            { refs = (w.refs != nullptr) ? w.refs : new std::atomic<int32_t>, *refs += 1; }                                                                                                                                                                 



            ~shr()                                                                                                                  

            {                                                                                                                               

                if (is_last())                                                                                                          

                {                                                                                                                               

                    delete this->obj; this->obj = nullptr;                                                                                  

                    delete refs; refs = nullptr;                                                                                    

                }                   

                else *refs -= 1;                                                                                                

            }                                                                                                                                                                                                                                               



            void operator = (T * obj)                                                                                               

            {                                                                                                                               

                this->obj = obj;                                                                                                        

                *refs = 1;                                                                                                      

            }                                                                                                                                                                                                                                               



            void operator = (const shr<T> & s)                                                                                      

            {                                                                                                                               

                this->obj = s.obj;                                                                                                      

                refs = (s.refs != nullptr) ? s.refs : new std::atomic<int32_t>, *refs += 1;                                     

            }                                                                                                                                                                                                                                               



            void operator = (const weak<T> & w)                                                                                     

            {                                                                                                                               

                this->obj = w.obj;                                                                                                      

                refs = (w.refs != nullptr) ? w.refs : new std::atomic<int32_t>, *refs += 1;                                     

            }                                                                                                                                                                                                                                               



            void reset()                                                                                                            

            {                                                                                                                               

                if (is_last())                                                                                                          

                {                                                                                                                               

                    delete this->obj; this->obj = nullptr;                                                                                  

                    delete refs; refs = nullptr;                                                                                    

                }                                                                                                                       

                else                                                                                                                    

                {                                                                                                                               

                    this->obj = nullptr;                                                                                                    

                    *refs -= 1; refs = nullptr;                                                                                     

                }                                                                                                               

            }       



            void reset(T * obj)                                                                                                     

            {                                                                                                                               

                if (is_last()) { delete this->obj; delete refs; }                                                                       

                else *refs -= 1;                                                                                                                                                                                                                                



                this->obj = obj;                                                                                                        

                refs = new std::atomic<int32_t>, *refs = 1;                                                                     

            }     



            inline const int32_t use_count() { return static_cast<int32_t>(*refs); }                                                

            inline bool unique() { return (*refs == 1); }                                                                           

            inline T * operator -> () { return this->obj; } 

            inline T & operator * () { return *(this->obj); }                                                       

    };                                                                                                                                                                                                                                              



    template<typename T>                                                                                                    

    class weak : public base<T>                                                                                             

    {                                                                                                                               

        private:                                                                                                                        

            friend class shr<T>;                                                                                                                                                                                                                            



            mutable std::atomic<int32_t> * refs;                                                                                                                                                                                                    



        public:                                                                                                                         

            weak() {}                                                                                                               

            weak(T * obj) : base<T>(obj) {}                                                                                         

            weak(const weak<T> & w) : base<T>(w->obj) {}                                                                            

            weak(const shr<T> & s) : base<T>(s.obj), refs(s.refs) {}



            void operator = (T * obj) { this->obj = obj; }                                                                          

            void operator = (const shr<T> & s) { this->obj = s.obj; refs = s.refs; }                                                void operator = (const weak<T> & w) { this->obj = w.obj; refs = w.refs; }                                                                                                                                                                       

            void reset() { this->obj = nullptr; refs = nullptr; }                                                                   

            void reset(T * obj) { this->obj = obj; refs = new std::atomic<int32_t>; *refs = 0; }                                                                                                                                                            



            inline shr<T> lock() { return shr<T>(this->obj); }                                                                      

            inline bool expired() { return ((refs == nullptr || *refs <= 0) ? true : false); }                                      

            inline const int32_t use_count() { return ((expired()) ? 0 : static_cast<int32_t>(*refs)); }            

    };                                                                                                                                                                                                                                              



    template<typename T>                                                                                                    

    const shr<T> make_shr(T * obj) { return shr<T>(obj); }                                                                                                                                                                                          



    template<typename T>                                                                                                    

    const unq<T> make_unq(T * obj) { return unq<T>(obj); }                                                          

}                                                                                                                                                                                                                                               

#endif                                                                                

I'll hit the red flags first, and then review the details.

template<typename T>                                                                                                    

const shr<T> make_shr(T * obj) { return shr<T>(obj); }

"Returning by const value" is a red flag. It doesn't do anything except occasionally disable move semantics. So at least we remove the const. But also, where there's one bug there's two. So we probably compare your make_shr<T>(...) to the Standard Library's make_shared<T>(...) and find out that your code does something vastly different. Consider

std::shared_ptr<int> sp = std::make_shared<int>(0);

assert(sp != nullptr);

ptr::shr<int> ps = ptr::make_shr<int>(0);

assert(ps == nullptr);

Well, actually I don't think ps == nullptr even compiles with your version, because your comparison operators only ever take base<T>, and the implicit conversion from nullptr_t to base<T> is protected so normal code can't use it. You should have a public conversion from std::nullptr_t, and it should express the idea that you don't take ownership of "null"; it's a special state without an owned object.

base(T * obj) : obj(obj) {}   

Each constructor should be explicit unless your goal is specifically to add an implicit conversion. Make this one explicit.

std::ostream & operator << (std::ostream & stream) { return (std::cout << obj); }

The red flag here is that this operator is completely backwards and broken. What you meant was

friend std::ostream& operator<< (std::ostream& stream, const base<T>& p) {

    stream << p.get();

    return stream;

}

Always use ADL friend functions to implement your operators (except for the few that have to be member functions, such as operator++).

Where there's one bug there's two (or more).

• Your version was streaming to std::cout regardless of which stream was passed in.


• "If it's not tested, it doesn't work." Even the very simplest test case would have shown that the version you wrote wasn't functional. If you don't plan to write tests for (or: don't plan to use) a feature, such as operator<<, then you might as well save time and just not write the feature!


• 
  

  Iostreams sucks: Even my new version is broken for ptr::shr<char>. I mean, std::cout << my_shr_ptr_to_char will end up calling operator<<(ostream&, char*), which will not print the pointer — it'll print the thing it points to, treated as a C string, which it almost certainly isn't. So it'll segfault and die. The simplest way to work around that is to make sure our code controls the exact overload of operator<< that we call: don't let it be template-dependent. So:

  
  
  

  friend std::ostream& operator<< (std::ostream& stream, const base<T>& p) {
  
      stream << static_cast<void*>(p.get());
  
      return stream;
  
  }
  
  

  
  

operator bool const () { return (obj != nullptr) ? true : false; }

This is a sneaky one I'd never seen before! You put the const in front of the () instead of behind it; so, this is another example of "returning by const value." What you meant to type was

operator bool () const { return (obj != nullptr) ? true : false; }

that is, a const member function (which promises not to modify the this object), that returns (non-const-qualified) bool.

Stylistically, there's no sense in writing condition ? true : false — that's like saying if condition is true, return true; if condition is false, return false. So:

operator bool () const { return (obj != nullptr); }

inline T * get() { return obj; }

Any time a function promises not to modify one of its reference parameters, you should make sure to const-qualify that parameter's referent. So, void f(int *p) is saying it might modify *p; void f(const int *p) is saying it promises not to modify *p. Similarly for any member function that promises not to modify its *this parameter: void mf() is saying it might modify *this; void mf() const is saying it promises not to modify *this.

T *get() const { return obj; }

I also removed the inline keyword because it wasn't doing anything. Functions defined in the body of a class, like this, Java/Python-style, are already inline by default. The only time you need inline is when you want to define a function in a header file but outside the body of a class.

That's enough red flags. Let me mention one super bug and then I'll call it a night.

class weak : public base<T> {

[...]

    mutable std::atomic<int32_t> * refs;

[...]

    [no destructor declared]

};

Having an RAII type like weak without a destructor is an oxymoron. weak must have a destructor to clean up its refs member, or else you'll have a leak.
(Also, refs doesn't need to be mutable.)

But wait, does weak even own its refs at all? Its constructor doesn't call new, so maybe it's okay that its destructor doesn't call delete?... Right. weak::refs is always initialized to point the same place as some shr's refs pointer. weak::refs is just an observer; shr::refs is the owner of the atomic<int32_t>.

But any time we have a non-owning observer, we should think about dangling. Can weak::refs dangle? Yes, it certainly can!

ptr::shr<int> p(new int(42));

ptr::weak<int> w(p);

p.reset();

w.expired();  // segfault

ptr::shr<int> q(w.lock());

assert(q != ptr::shr<int>(nullptr));

*q;  // segfault

But your weak is all screwed up. Since it's unusable, you should just remove it. Bring it back if you ever run into a case where you need to use something like weak_ptr, so that you have some idea of what the requirements are. (For example, "locking an expired weak_ptr should return null," or "locking an unexpired weak_ptr should increment the original refcount, not create a new refcount competing with the first," or "it is nonsensical to create a weak_ptr from a raw T*."

Write some test cases for your ptr::unq and ptr::shr. You'll find bugs. Think about how to fix those bugs, and then (only then!) fix them. As you improve your understanding of what the code needs to do, you'll improve your coding style as well.

Let's have a look at some examples where it fails.

Rule of Three

You have not correctly over written the assignment operator.

ptr::unq<int>   x(new int(5));

ptr::unq<int>   y;



y = x;   // This is broken. You should look up rule of three.

The above code compiles. BUT is broken. This pointer will get deleted twice. In debug mode on my compiler it even shows this.

> ./a.out

a.out(7619,0x10f20c5c0) malloc: *** error for object 0x7ff597c02ac0: pointer 

being freed was not allocated

a.out(7619,0x10f20c5c0) malloc: *** set a breakpoint in malloc_error_break to debug

>

Rule of Five

Now I try and use the move operators.

ptr::unq<int>   x(new int(5));

ptr::unq<int>   y;



y = std::move(x);   // This compiles. Which is a surprise.

But again when we run the code and generate an error.

> ./a.out

a.out(7619,0x10f20c5c0) malloc: *** error for object 0x7ff597c02ac0: pointer 

being freed was not allocated

a.out(7619,0x10f20c5c0) malloc: *** set a breakpoint in malloc_error_break to debug

>

This suggests that something not quite correct is happening.

Implicit construction

You have an implicit construction problem.

Imagine this situation:

void doWork(ptr::unq<int> data)

{

    std::cout << "Do Workn";

}



int main()

{

    int*  x = new int(5);

    doWork(x);            // This creates a ptr::unq<int> object.

                          // This object is destroyed at the call which will

                          // call delete on the pointer passed.



    delete x;             // This means this is an extra delete on the pointer

                          // which makes it a bug.

}

Running this we get:

> ./a.out

a.out(7619,0x10f20c5c0) malloc: *** error for object 0x7ff597c02ac0: pointer 

being freed was not allocated

a.out(7619,0x10f20c5c0) malloc: *** set a breakpoint in malloc_error_break to debug

>

I like that you added a bool operators

        operator bool const () { return (obj != nullptr) ? true : false; }  

Couple of things wrong:

• The const is in the wrong place.


• The test is a bit verbose. You are testing a boolean expression (obj != nullptr) then using a trinary operator to extract that value, much easier to simply return the expression.


• 
  

  You also need to use explicit. Otherwise we can use the comparison to compare pointers in a way that we do not intend.

  
  
  

  ptr::unq<int>    uniqueInt(new int(5));
  
  ptr::unq<flt>    uniqueFlt(new flt(12.0));
  
  
  
  if (uniqueInt == uniqueFlt) {
  
      std::cout << "I bet this printsn";
  
  }
  
  

  
  

Now when I run:

 > ./a.out

 I bet this prints

 >

To prevent this you should tack on explicit. This prevents unrequired conversions.

 explicit operator bool () const { return obj != nullptr; }