Generic Property class

Question

Some time ago I wrote a little property class for a then abandoned project. I just stumbled over it again and I would like to know if the design is using good, efficient C++.

#pragma once
#ifndef NO_THREAD_SAFETY
#include <atomic>
#endif
template<typename T1>
class Property {
public:
#ifndef NO_THREAD_SAFETY
 typedef std::atomic<long> PropertyID;
 typedef std::atomic<T1> PropertyValue;
#else
 typedef long PropertyID;
 typedef T1 PropertyValue;
#endif
 Property(T1 value): value(value), id(id_count++) {
 }
 Property(): value(), id(id_count++) {
 }
 Property(const Property &rhs): id(id_count++) {
 value = static_cast<T1>(rhs.value);
 }
 Property(Property &&rhs): value(std::move(rhs.value)), id(id_count++) {
 }
 const Property &operator=(const Property &rhs) {
 value = std::move(static_cast<T1>(rhs.value));
 return *this;
 }
 const Property &operator=(const T1 &rhs) {
 value = rhs;
 return *this;
 }
 const Property &operator=(T1 &&rhs) {
 value = std::move(rhs);
 return *this;
 }
 bool operator==(const Property &rhs) const {
 return value == rhs.value;
 }
 bool operator==(const T1 &rhs) const {
 return value == rhs;
 }
 bool operator< (const Property &rhs) const {
 return value < rhs.value;
 }
 bool operator< (const T1 &rhs) const {
 return value < rhs;
 }
 void set(const T1 &value) {
 this->value = value;
 }
 T1 get() const {
 return value;
 }
 operator T1() const {
 return value;
 }
 PropertyID getID() const {
 return id;
 }
private:
 PropertyValue value;
 PropertyID id;
 static PropertyID id_count;
};
template<typename T1>
typename Property<T1>::PropertyID Property<T1>::id_count (0);

Answer 1

These are the things that stuck out to me:

1. Your copy constructor and move constructor don't result in an identical Property<T1> as they increment the id. Why is that? (There is no documentation anywhere in this that helps the consumer of your Property class.)
2. Your value constructor Property(T1 value) should probably initialize value with std::move(value). It has already received a copy (perhaps moved), so it is safe to move from value.
3. Your copy constructor uses what appears to be a superfluous static_cast<T1> and uses the constructor body instead of an initialization list. I would suggest this be more like the move constructor just without the call to std::move.

4. As nothing appears to change the value of id after it has been constructed, I don't see the benefit of id being atomic. I do understand the value of id_count being atomic, but making them both atomic seems to be asking for unnecessary overhead. Related, perhaps the usage of id_count should be moved to a static function so that it cannot be subverted.

   static PropertyID getNextId() { // assumes PropertyID is non-atomic
    static AtomicPropertyID id = 0; // and AtomicPropertyID is conditionally atomic
    return id++;
   }
   

5. I don't quite understand how Property<T1> is expected to be used. Does it go in a collection of properties? Is it used to declare as members of another type? If the former, the implementation of operator< doesn't seem to provide any useful characteristics for finding the right property. If the latter, the encapsulation seems to be a very long winded way of conditionally making a member atomic - since there is no business logic in the setter or getter methods, and no way to provide that other than inheriting, I'm unclear what the benefit is of using this class.

• Appleshell: Thanks! Very helpful answer. I'll just comment on a few things: 1) Copying should result in a new ID because the copied property can be altered independently. Moving of course shouldn't. 3) I can't remember why I did this in the first place! 4) id being atomic was indeed unnecessary. 5) I don't know why I wrote it in the old project, but such class can be useful when adding serialization specializations, or adding callbacks for when values are changed.