C++11 smart pointer 'library'
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 3 hours ago
sjh919
132
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 |
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 3 hours ago
sjh919
132
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 |
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 3 hours ago
sjh919
132
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.
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 3 hours ago
sjh919
132
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 3 hours ago
sjh919
132
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 3 hours ago
sjh919
132
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 3 hours ago
sjh919
132
asked 3 hours ago
sjh919
132
132
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.
add a comment |
add a comment |
1 Answer
1
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& p) {
stream << static_cast(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.
answered 2 hours ago
Quuxplusone
11.1k11858
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 hours 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 hours ago
"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
1 hour 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
1 hour ago
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
1 Answer
1
active
oldest
votes
1 Answer
1
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& p) {
stream << static_cast(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.
answered 2 hours ago
Quuxplusone
11.1k11858
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 hours 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 hours ago
"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
1 hour 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
1 hour ago
add a comment |
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& p) {
stream << static_cast(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.
answered 2 hours ago
Quuxplusone
11.1k11858
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 hours 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 hours ago
"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
1 hour 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
1 hour ago
add a comment |
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& p) {
stream << static_cast(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.
answered 2 hours ago
Quuxplusone
11.1k11858
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& p) {
stream << static_cast(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.
answered 2 hours ago
Quuxplusone
11.1k11858
answered 2 hours ago
Quuxplusone
11.1k11858
answered 2 hours ago
Quuxplusone
11.1k11858
answered 2 hours ago
Quuxplusone
11.1k11858
11.1k11858
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 hours 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 hours ago
"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
1 hour 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
1 hour ago
add a comment |
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 hours 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 hours ago
"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
1 hour 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
1 hour 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 hours 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 hours 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 hours 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 hours 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
1 hour 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
1 hour 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
1 hour 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
1 hour ago
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
