C++11 smart pointer 'library'
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
c++ c++11 pointers
asked 2 days ago
sjh919
685
New contributor
sjh919 is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.
add a comment |
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
c++ c++11 pointers
asked 2 days ago
sjh919
685
New contributor
sjh919 is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.
Quite a lot missing Have a look here And here and constructors
– Martin York
2 days ago
add a comment |
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
c++ c++11 pointers
asked 2 days ago
sjh919
685
New contributor
sjh919 is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.
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
c++ c++11 pointers
c++ c++11 pointers
asked 2 days ago
sjh919
685
New contributor
sjh919 is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.
asked 2 days ago
sjh919
685
New contributor
sjh919 is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.
edited yesterday
asked 2 days ago
sjh919
685
New contributor
sjh919 is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.
asked 2 days ago
sjh919
685
asked 2 days ago
sjh919
685
685
New contributor
sjh919 is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.
New contributor
sjh919 is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.
sjh919 is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.
Quite a lot missing Have a look here And here and constructors
– Martin York
2 days ago
add a comment |
Quite a lot missing Have a look here And here and constructors
– Martin York
2 days ago
Quite a lot missing Have a look here And here and constructors
– Martin York
2 days ago
Quite a lot missing Have a look here And here and constructors
– Martin York
2 days ago
add a comment |
2 Answers
2
active
oldest
votes
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::coutregardless of whichstreamwas 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_charwill end up callingoperator<<(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 ofoperator<<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.
edited 2 days ago
hoffmale
5,567835
answered 2 days ago
Quuxplusone
11.3k11959
1
Thanks a ton for the review! And yea I had a little trouble wrapping my head around how std::weak worked and my implementation here is the result of that. Do you have any tips for effectively implementing it if I try to keep it?
– sjh919
2 days ago
Also, any critiques on how the program is structured overall? I feel like I could've done it a bit more efficiently but not exactly sure where. Other sidenote (sorry), I also used mutable for reference counters when taking in const arguments in the constructors, not sure if there is another way around that or if just remove the 'const' altogether?
– sjh919
2 days ago
3
"Do you have any tips for effectively implementing [weak_ptr]?" Sort of. Does "buy my book" count as a tip? ;)weak_ptris explained, with diagrams and source code, in Chapter 6.
– Quuxplusone
2 days ago
"I also used mutable for reference counters when taking in const arguments in the constructors" — This comment makes no sense to me. Just remove themutableand recompile; it'll be fine.
– Quuxplusone
2 days ago
2
@sjh919: Note that there are a lot more dangling/memory leak issues than the ones mentioned in this answer (e.g.auto ptr = ptr::unq<int>(new int); ptr = new int;leaks the old value and willdeletethe new one two times!). Also, why have a (non-working) copy contructor forptr::unq? // @Quuxplusone: You might want to add a small section on the other parts of the rule of five ;)
– hoffmale
2 days ago
|
show 2 more comments
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
constis 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; }
answered 2 days ago
Martin York
72.5k483261
Thank you! Didn't notice the assignment bug with unq. I did notice in your linked article that you mentioned making a smart pointer implementation for learning is a bad idea because it is hard to get it correct in all contexts. While I somewhat agree (in hindsight, after seeing all of my errors) I purposefully made this a subset of a full implementation, where this is in no way intended to be the full thing. Would you agree this is fine to do? Further, other than the specific errors mentioned so far, do you have any critiques of the overall structure of the implementation? Thanks!
– sjh919
yesterday
@sjh919 I think writing a smart pointers is a good exercise for any programmer as it teaches a lot. BUT I think it is also very hard and something you should try once you have a lot of experience already. I think it is way too difficult for a beginner.
– Martin York
yesterday
add a comment |
Your Answer
StackExchange.ifUsing("editor", function () {
return StackExchange.using("mathjaxEditing", function () {
StackExchange.MarkdownEditor.creationCallbacks.add(function (editor, postfix) {
StackExchange.mathjaxEditing.prepareWmdForMathJax(editor, postfix, [["\$", "\$"]]);
});
});
}, "mathjax-editing");
StackExchange.ifUsing("editor", function () {
StackExchange.using("externalEditor", function () {
StackExchange.using("snippets", function () {
StackExchange.snippets.init();
});
});
}, "code-snippets");
StackExchange.ready(function() {
var channelOptions = {
tags: "".split(" "),
id: "196"
};
initTagRenderer("".split(" "), "".split(" "), channelOptions);
StackExchange.using("externalEditor", function() {
// Have to fire editor after snippets, if snippets enabled
if (StackExchange.settings.snippets.snippetsEnabled) {
StackExchange.using("snippets", function() {
createEditor();
});
}
else {
createEditor();
}
});
function createEditor() {
StackExchange.prepareEditor({
heartbeatType: 'answer',
autoActivateHeartbeat: false,
convertImagesToLinks: false,
noModals: true,
showLowRepImageUploadWarning: true,
reputationToPostImages: null,
bindNavPrevention: true,
postfix: "",
imageUploader: {
brandingHtml: "Powered by u003ca class="icon-imgur-white" href="https://imgur.com/"u003eu003c/au003e",
contentPolicyHtml: "User contributions licensed under u003ca href="https://creativecommons.org/licenses/by-sa/3.0/"u003ecc by-sa 3.0 with attribution requiredu003c/au003e u003ca href="https://stackoverflow.com/legal/content-policy"u003e(content policy)u003c/au003e",
allowUrls: true
},
onDemand: true,
discardSelector: ".discard-answer"
,immediatelyShowMarkdownHelp:true
});
}
});
sjh919 is a new contributor. Be nice, and check out our Code of Conduct.
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
StackExchange.ready(
function () {
StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f210557%2fc11-smart-pointer-library%23new-answer', 'question_page');
}
);
Post as a guest
Required, but never shown
2 Answers
2
active
oldest
votes
2 Answers
2
active
oldest
votes
active
oldest
votes
active
oldest
votes
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::coutregardless of whichstreamwas 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_charwill end up callingoperator<<(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 ofoperator<<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.
edited 2 days ago
hoffmale
5,567835
answered 2 days ago
Quuxplusone
11.3k11959
1
Thanks a ton for the review! And yea I had a little trouble wrapping my head around how std::weak worked and my implementation here is the result of that. Do you have any tips for effectively implementing it if I try to keep it?
– sjh919
2 days ago
Also, any critiques on how the program is structured overall? I feel like I could've done it a bit more efficiently but not exactly sure where. Other sidenote (sorry), I also used mutable for reference counters when taking in const arguments in the constructors, not sure if there is another way around that or if just remove the 'const' altogether?
– sjh919
2 days ago
3
"Do you have any tips for effectively implementing [weak_ptr]?" Sort of. Does "buy my book" count as a tip? ;)weak_ptris explained, with diagrams and source code, in Chapter 6.
– Quuxplusone
2 days ago
"I also used mutable for reference counters when taking in const arguments in the constructors" — This comment makes no sense to me. Just remove themutableand recompile; it'll be fine.
– Quuxplusone
2 days ago
2
@sjh919: Note that there are a lot more dangling/memory leak issues than the ones mentioned in this answer (e.g.auto ptr = ptr::unq<int>(new int); ptr = new int;leaks the old value and willdeletethe new one two times!). Also, why have a (non-working) copy contructor forptr::unq? // @Quuxplusone: You might want to add a small section on the other parts of the rule of five ;)
– hoffmale
2 days ago
|
show 2 more comments
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::coutregardless of whichstreamwas 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_charwill end up callingoperator<<(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 ofoperator<<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.
edited 2 days ago
hoffmale
5,567835
answered 2 days ago
Quuxplusone
11.3k11959
1
Thanks a ton for the review! And yea I had a little trouble wrapping my head around how std::weak worked and my implementation here is the result of that. Do you have any tips for effectively implementing it if I try to keep it?
– sjh919
2 days ago
Also, any critiques on how the program is structured overall? I feel like I could've done it a bit more efficiently but not exactly sure where. Other sidenote (sorry), I also used mutable for reference counters when taking in const arguments in the constructors, not sure if there is another way around that or if just remove the 'const' altogether?
– sjh919
2 days ago
3
"Do you have any tips for effectively implementing [weak_ptr]?" Sort of. Does "buy my book" count as a tip? ;)weak_ptris explained, with diagrams and source code, in Chapter 6.
– Quuxplusone
2 days ago
"I also used mutable for reference counters when taking in const arguments in the constructors" — This comment makes no sense to me. Just remove themutableand recompile; it'll be fine.
– Quuxplusone
2 days ago
2
@sjh919: Note that there are a lot more dangling/memory leak issues than the ones mentioned in this answer (e.g.auto ptr = ptr::unq<int>(new int); ptr = new int;leaks the old value and willdeletethe new one two times!). Also, why have a (non-working) copy contructor forptr::unq? // @Quuxplusone: You might want to add a small section on the other parts of the rule of five ;)
– hoffmale
2 days ago
|
show 2 more comments
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::coutregardless of whichstreamwas 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_charwill end up callingoperator<<(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 ofoperator<<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.
edited 2 days ago
hoffmale
5,567835
answered 2 days ago
Quuxplusone
11.3k11959
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::coutregardless of whichstreamwas 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_charwill end up callingoperator<<(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 ofoperator<<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.
edited 2 days ago
hoffmale
5,567835
answered 2 days ago
Quuxplusone
11.3k11959
edited 2 days ago
hoffmale
5,567835
edited 2 days ago
hoffmale
5,567835
edited 2 days ago
hoffmale
5,567835
5,567835
answered 2 days ago
Quuxplusone
11.3k11959
answered 2 days ago
Quuxplusone
11.3k11959
answered 2 days ago
Quuxplusone
11.3k11959
11.3k11959
1
Thanks a ton for the review! And yea I had a little trouble wrapping my head around how std::weak worked and my implementation here is the result of that. Do you have any tips for effectively implementing it if I try to keep it?
– sjh919
2 days ago
Also, any critiques on how the program is structured overall? I feel like I could've done it a bit more efficiently but not exactly sure where. Other sidenote (sorry), I also used mutable for reference counters when taking in const arguments in the constructors, not sure if there is another way around that or if just remove the 'const' altogether?
– sjh919
2 days ago
3
"Do you have any tips for effectively implementing [weak_ptr]?" Sort of. Does "buy my book" count as a tip? ;)weak_ptris explained, with diagrams and source code, in Chapter 6.
– Quuxplusone
2 days ago
"I also used mutable for reference counters when taking in const arguments in the constructors" — This comment makes no sense to me. Just remove themutableand recompile; it'll be fine.
– Quuxplusone
2 days ago
2
@sjh919: Note that there are a lot more dangling/memory leak issues than the ones mentioned in this answer (e.g.auto ptr = ptr::unq<int>(new int); ptr = new int;leaks the old value and willdeletethe new one two times!). Also, why have a (non-working) copy contructor forptr::unq? // @Quuxplusone: You might want to add a small section on the other parts of the rule of five ;)
– hoffmale
2 days ago
|
show 2 more comments
1
Thanks a ton for the review! And yea I had a little trouble wrapping my head around how std::weak worked and my implementation here is the result of that. Do you have any tips for effectively implementing it if I try to keep it?
– sjh919
2 days ago
Also, any critiques on how the program is structured overall? I feel like I could've done it a bit more efficiently but not exactly sure where. Other sidenote (sorry), I also used mutable for reference counters when taking in const arguments in the constructors, not sure if there is another way around that or if just remove the 'const' altogether?
– sjh919
2 days ago
3
"Do you have any tips for effectively implementing [weak_ptr]?" Sort of. Does "buy my book" count as a tip? ;)weak_ptris explained, with diagrams and source code, in Chapter 6.
– Quuxplusone
2 days ago
"I also used mutable for reference counters when taking in const arguments in the constructors" — This comment makes no sense to me. Just remove themutableand recompile; it'll be fine.
– Quuxplusone
2 days ago
2
@sjh919: Note that there are a lot more dangling/memory leak issues than the ones mentioned in this answer (e.g.auto ptr = ptr::unq<int>(new int); ptr = new int;leaks the old value and willdeletethe new one two times!). Also, why have a (non-working) copy contructor forptr::unq? // @Quuxplusone: You might want to add a small section on the other parts of the rule of five ;)
– hoffmale
2 days ago
1
1
Thanks a ton for the review! And yea I had a little trouble wrapping my head around how std::weak worked and my implementation here is the result of that. Do you have any tips for effectively implementing it if I try to keep it?
– sjh919
2 days ago
Thanks a ton for the review! And yea I had a little trouble wrapping my head around how std::weak worked and my implementation here is the result of that. Do you have any tips for effectively implementing it if I try to keep it?
– sjh919
2 days ago
Also, any critiques on how the program is structured overall? I feel like I could've done it a bit more efficiently but not exactly sure where. Other sidenote (sorry), I also used mutable for reference counters when taking in const arguments in the constructors, not sure if there is another way around that or if just remove the 'const' altogether?
– sjh919
2 days ago
Also, any critiques on how the program is structured overall? I feel like I could've done it a bit more efficiently but not exactly sure where. Other sidenote (sorry), I also used mutable for reference counters when taking in const arguments in the constructors, not sure if there is another way around that or if just remove the 'const' altogether?
– sjh919
2 days ago
3
3
"Do you have any tips for effectively implementing [
weak_ptr]?" Sort of. Does "buy my book" count as a tip? ;) weak_ptr is explained, with diagrams and source code, in Chapter 6.– Quuxplusone
2 days ago
"Do you have any tips for effectively implementing [
weak_ptr]?" Sort of. Does "buy my book" count as a tip? ;) weak_ptr is explained, with diagrams and source code, in Chapter 6.– Quuxplusone
2 days ago
"I also used mutable for reference counters when taking in const arguments in the constructors" — This comment makes no sense to me. Just remove the
mutable and recompile; it'll be fine.– Quuxplusone
2 days ago
"I also used mutable for reference counters when taking in const arguments in the constructors" — This comment makes no sense to me. Just remove the
mutable and recompile; it'll be fine.– Quuxplusone
2 days ago
2
2
@sjh919: Note that there are a lot more dangling/memory leak issues than the ones mentioned in this answer (e.g.
auto ptr = ptr::unq<int>(new int); ptr = new int; leaks the old value and will delete the new one two times!). Also, why have a (non-working) copy contructor for ptr::unq? // @Quuxplusone: You might want to add a small section on the other parts of the rule of five ;)– hoffmale
2 days ago
@sjh919: Note that there are a lot more dangling/memory leak issues than the ones mentioned in this answer (e.g.
auto ptr = ptr::unq<int>(new int); ptr = new int; leaks the old value and will delete the new one two times!). Also, why have a (non-working) copy contructor for ptr::unq? // @Quuxplusone: You might want to add a small section on the other parts of the rule of five ;)– hoffmale
2 days ago
|
show 2 more comments
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
constis 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; }
answered 2 days ago
Martin York
72.5k483261
Thank you! Didn't notice the assignment bug with unq. I did notice in your linked article that you mentioned making a smart pointer implementation for learning is a bad idea because it is hard to get it correct in all contexts. While I somewhat agree (in hindsight, after seeing all of my errors) I purposefully made this a subset of a full implementation, where this is in no way intended to be the full thing. Would you agree this is fine to do? Further, other than the specific errors mentioned so far, do you have any critiques of the overall structure of the implementation? Thanks!
– sjh919
yesterday
@sjh919 I think writing a smart pointers is a good exercise for any programmer as it teaches a lot. BUT I think it is also very hard and something you should try once you have a lot of experience already. I think it is way too difficult for a beginner.
– Martin York
yesterday
add a comment |
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
constis 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; }
answered 2 days ago
Martin York
72.5k483261
Thank you! Didn't notice the assignment bug with unq. I did notice in your linked article that you mentioned making a smart pointer implementation for learning is a bad idea because it is hard to get it correct in all contexts. While I somewhat agree (in hindsight, after seeing all of my errors) I purposefully made this a subset of a full implementation, where this is in no way intended to be the full thing. Would you agree this is fine to do? Further, other than the specific errors mentioned so far, do you have any critiques of the overall structure of the implementation? Thanks!
– sjh919
yesterday
@sjh919 I think writing a smart pointers is a good exercise for any programmer as it teaches a lot. BUT I think it is also very hard and something you should try once you have a lot of experience already. I think it is way too difficult for a beginner.
– Martin York
yesterday
add a comment |
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
constis 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; }
answered 2 days ago
Martin York
72.5k483261
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
constis 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; }
answered 2 days ago
Martin York
72.5k483261
answered 2 days ago
Martin York
72.5k483261
answered 2 days ago
Martin York
72.5k483261
answered 2 days ago
Martin York
72.5k483261
72.5k483261
Thank you! Didn't notice the assignment bug with unq. I did notice in your linked article that you mentioned making a smart pointer implementation for learning is a bad idea because it is hard to get it correct in all contexts. While I somewhat agree (in hindsight, after seeing all of my errors) I purposefully made this a subset of a full implementation, where this is in no way intended to be the full thing. Would you agree this is fine to do? Further, other than the specific errors mentioned so far, do you have any critiques of the overall structure of the implementation? Thanks!
– sjh919
yesterday
@sjh919 I think writing a smart pointers is a good exercise for any programmer as it teaches a lot. BUT I think it is also very hard and something you should try once you have a lot of experience already. I think it is way too difficult for a beginner.
– Martin York
yesterday
add a comment |
Thank you! Didn't notice the assignment bug with unq. I did notice in your linked article that you mentioned making a smart pointer implementation for learning is a bad idea because it is hard to get it correct in all contexts. While I somewhat agree (in hindsight, after seeing all of my errors) I purposefully made this a subset of a full implementation, where this is in no way intended to be the full thing. Would you agree this is fine to do? Further, other than the specific errors mentioned so far, do you have any critiques of the overall structure of the implementation? Thanks!
– sjh919
yesterday
@sjh919 I think writing a smart pointers is a good exercise for any programmer as it teaches a lot. BUT I think it is also very hard and something you should try once you have a lot of experience already. I think it is way too difficult for a beginner.
– Martin York
yesterday
Thank you! Didn't notice the assignment bug with unq. I did notice in your linked article that you mentioned making a smart pointer implementation for learning is a bad idea because it is hard to get it correct in all contexts. While I somewhat agree (in hindsight, after seeing all of my errors) I purposefully made this a subset of a full implementation, where this is in no way intended to be the full thing. Would you agree this is fine to do? Further, other than the specific errors mentioned so far, do you have any critiques of the overall structure of the implementation? Thanks!
– sjh919
yesterday
Thank you! Didn't notice the assignment bug with unq. I did notice in your linked article that you mentioned making a smart pointer implementation for learning is a bad idea because it is hard to get it correct in all contexts. While I somewhat agree (in hindsight, after seeing all of my errors) I purposefully made this a subset of a full implementation, where this is in no way intended to be the full thing. Would you agree this is fine to do? Further, other than the specific errors mentioned so far, do you have any critiques of the overall structure of the implementation? Thanks!
– sjh919
yesterday
@sjh919 I think writing a smart pointers is a good exercise for any programmer as it teaches a lot. BUT I think it is also very hard and something you should try once you have a lot of experience already. I think it is way too difficult for a beginner.
– Martin York
yesterday
@sjh919 I think writing a smart pointers is a good exercise for any programmer as it teaches a lot. BUT I think it is also very hard and something you should try once you have a lot of experience already. I think it is way too difficult for a beginner.
– Martin York
yesterday
add a comment |
sjh919 is a new contributor. Be nice, and check out our Code of Conduct.
sjh919 is a new contributor. Be nice, and check out our Code of Conduct.
sjh919 is a new contributor. Be nice, and check out our Code of Conduct.
sjh919 is a new contributor. Be nice, and check out our Code of Conduct.
Thanks for contributing an answer to Code Review Stack Exchange!
- Please be sure to answer the question. Provide details and share your research!
But avoid …
- Asking for help, clarification, or responding to other answers.
- Making statements based on opinion; back them up with references or personal experience.
Use MathJax to format equations. MathJax reference.
To learn more, see our tips on writing great answers.
Some of your past answers have not been well-received, and you're in danger of being blocked from answering.
Please pay close attention to the following guidance:
- Please be sure to answer the question. Provide details and share your research!
But avoid …
- Asking for help, clarification, or responding to other answers.
- Making statements based on opinion; back them up with references or personal experience.
To learn more, see our tips on writing great answers.
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
StackExchange.ready(
function () {
StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f210557%2fc11-smart-pointer-library%23new-answer', 'question_page');
}
);
Post as a guest
Required, but never shown
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Quite a lot missing Have a look here And here and constructors
– Martin York
2 days ago