Skip to content

Add move assignment operator for Json::Value class and overload append member function for RValue references #635

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from Jul 11, 2017
Merged

Add move assignment operator for Json::Value class and overload append member function for RValue references #635

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8996c37
add move assignment operator for Json::Value class.
dhruvCW Jul 8, 2017
23c44d9
overload append function for R value references.
dhruvCW Jul 8, 2017
0ba8bd7
add move assignment operator for CZString and change copy assignment …
dhruvCW Jul 8, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 23 additions & 5 deletions include/json/value.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@
#endif

//Conditional NORETURN attribute on the throw functions would:
// a) suppress false positives from static code analysis
// a) suppress false positives from static code analysis
// b) possibly improve optimization opportunities.
#if !defined(JSONCPP_NORETURN)
# if defined(_MSC_VER)
Expand Down Expand Up @@ -64,7 +64,7 @@ class JSON_API Exception : public std::exception {
/** Exceptions which the user cannot easily avoid.
*
* E.g. out-of-memory (when we use malloc), stack-overflow, malicious input
*
*
* \remark derived from Json::Exception
*/
class JSON_API RuntimeError : public Exception {
Expand All @@ -75,7 +75,7 @@ class JSON_API RuntimeError : public Exception {
/** Exceptions thrown by JSON_ASSERT/JSON_FAIL macros.
*
* These are precondition-violations (user bugs) and internal errors (our bugs).
*
*
* \remark derived from Json::Exception
*/
class JSON_API LogicError : public Exception {
Expand Down Expand Up @@ -233,7 +233,12 @@ class JSON_API Value {
CZString(CZString&& other);
#endif
~CZString();
CZString& operator=(CZString other);
CZString& operator=(const CZString& other);

#if JSON_HAS_RVALUE_REFERENCES
CZString& operator=(CZString&& other);
#endif

bool operator<(CZString const& other) const;
bool operator==(CZString const& other) const;
ArrayIndex index() const;
Expand Down Expand Up @@ -322,12 +327,21 @@ Json::Value obj_value(Json::objectValue); // {}

/// Deep copy, then swap(other).
/// \note Over-write existing comments. To preserve comments, use #swapPayload().
Value& operator=(Value other);
Value& operator=(const Value& other);
#if JSON_HAS_RVALUE_REFERENCES
Value& operator=(Value&& other);
#endif

/// Swap everything.
void swap(Value& other);
/// Swap values but leave comments and source offsets in place.
void swapPayload(Value& other);

/// copy everything.
void copy(const Value& other);
/// copy values but leave comments and source offsets in place.
void copyPayload(const Value& other);

ValueType type() const;

/// Compare payload only, not comments etc.
Expand Down Expand Up @@ -438,6 +452,10 @@ Json::Value obj_value(Json::objectValue); // {}
/// Equivalent to jsonvalue[jsonvalue.size()] = value;
Value& append(const Value& value);

#if JSON_HAS_RVALUE_REFERENCES
Value& append(Value&& value);
#endif

/// Access an object value by name, create a null member if it does not exist.
/// \note Because of our implementation, keys are limited to 2^30 -1 chars.
/// Exceeding that will cause an exception.
Expand Down
43 changes: 39 additions & 4 deletions src/lib_json/json_value.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -292,11 +292,21 @@ void Value::CZString::swap(CZString& other) {
std::swap(index_, other.index_);
}

Value::CZString& Value::CZString::operator=(CZString other) {
swap(other);
Value::CZString& Value::CZString::operator=(const CZString& other) {
cstr_ = other.cstr_;
index_ = other.index_;
return *this;
}

#if JSON_HAS_RVALUE_REFERENCES
Value::CZString& Value::CZString::operator=(CZString&& other) {
cstr_ = other.cstr_;
index_ = other.index_;
other.cstr_ = nullptr;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Accordingly to https://docs.microsoft.com/en-us/cpp/cpp/move-constructors-and-move-assignment-operators-cpp?view=vs-2019

  1. You should zero the original objects after moving the value.
  2. Check if you are not moving *this into *this.
   // Copy the data pointer and its length from the
   // source object.
   _data = other._data;
   _length = other._length;

   // Release the data pointer from the source object so that
   // the destructor does not free the memory multiple times.
   other._data = nullptr;
   other._length = 0;

...
   if (this != &other)

Copy link
Contributor

@BillyDonahue BillyDonahue Oct 7, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the link. I am a little skeptical of some sloppiness in the article, but it's fine for beginners I guess.

You don't have to zero everything. Only do enough work to make ~T and T::operator= valid and enforce your postconditions regarding the moved-from state of your class, which is up to you. Moved-from object's destructor and reassignment need to be correct, and maybe nothing else.

You don't have to do check against self-move and do nothing. You only have to anticipate that self-move-assign is possible and prove that it leaves the object in a safe moved-from state. Adding these checks without thinking about it can slow down the operation, just to support an outlier case.

All that said! CZString is not safe against self-move-assign. It will leak its allocation.
That's a for-real bug.

Value::CZString& Value::CZString::operator=(CZString&& other) {
  cstr_ = other.cstr_;
  index_ = other.index_;
  other.cstr_ = nullptr;  // cstr_ is leaked if this is self-move-assignment
  return *this;
}

return *this;
}
#endif

bool Value::CZString::operator<(const CZString& other) const {
if (!cstr_) return index_ < other.index_;
//return strcmp(cstr_, other.cstr_) < 0;
Expand Down Expand Up @@ -398,7 +408,7 @@ Value::Value(double value) {

Value::Value(const char* value) {
initBasic(stringValue, true);
JSON_ASSERT_MESSAGE(value != NULL, "Null Value Passed to Value Constructor");
JSON_ASSERT_MESSAGE(value != NULL, "Null Value Passed to Value Constructor");
value_.string_ = duplicateAndPrefixStringValue(value, static_cast<unsigned>(strlen(value)));
}

Expand Down Expand Up @@ -508,10 +518,18 @@ Value::~Value() {
value_.uint_ = 0;
}

Value& Value::operator=(Value other) {
Value& Value::operator=(const Value& other) {
swap(const_cast<Value&>(other));
return *this;
}

#if JSON_HAS_RVALUE_REFERENCES
Value& Value::operator=(Value&& other) {
initBasic(nullValue);
swap(other);
return *this;
}
#endif

void Value::swapPayload(Value& other) {
ValueType temp = type_;
Expand All @@ -523,13 +541,26 @@ void Value::swapPayload(Value& other) {
other.allocated_ = temp2 & 0x1;
}

void Value::copyPayload(const Value& other) {
type_ = other.type_;
value_ = other.value_;
allocated_ = other.allocated_;
}

void Value::swap(Value& other) {
swapPayload(other);
std::swap(comments_, other.comments_);
std::swap(start_, other.start_);
std::swap(limit_, other.limit_);
}

void Value::copy(const Value& other) {
copyPayload(other);
comments_ = other.comments_;
start_ = other.start_;
limit_ = other.limit_;
}

ValueType Value::type() const { return type_; }

int Value::compare(const Value& other) const {
Expand Down Expand Up @@ -1124,6 +1155,10 @@ Value const& Value::operator[](CppTL::ConstString const& key) const

Value& Value::append(const Value& value) { return (*this)[size()] = value; }

#if JSON_HAS_RVALUE_REFERENCES
Value& Value::append(Value&& value) { return (*this)[size()] = value; }
#endif

Value Value::get(char const* key, char const* cend, Value const& defaultValue) const
{
Value const* found = find(key, cend);
Expand Down