Skip to content
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.

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

Templated accessors and range-based for #542

Merged
merged 18 commits into from Feb 19, 2016
Merged

Conversation

miloyip
Copy link
Collaborator

@miloyip miloyip commented Feb 14, 2016

Fix #316, #162, #212

Request for code review


template<typename ValueType>
struct TypeHelper<ValueType, typename ValueType::Array> {
typedef typename ValueType::Array ArratType;
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo here: ArratType?

@pah
Copy link
Contributor

pah commented Feb 16, 2016

👍 LGTM, thanks!

I haven't seen a conversion from Generic(Array|Object) back to GenericValue. Wouldn't this conversion be helpful to move the contents of an Array/Object back into another Value? The only challenge would be to restore the kArrayType|kObjectType invariant after moving from the nested Value. What do you think? Maybe, it's better to add additional (but here non-explicit) constructors to GenericValue?

@miloyip
Copy link
Collaborator Author

miloyip commented Feb 17, 2016

There are GenericValue::SetArray|Object(Array|Object&) already.
I do not understand

The only challenge would be to restore the kArrayType|kObjectType invariant after moving from the nested Value.

Maybe a (move) constructor and assignment will be more complete.
How about other similar operations such as PushBack(Object|Array), AddMember(...), etc?

@pah
Copy link
Contributor

pah commented Feb 17, 2016

There are GenericValue::SetArray|Object(Array|Object&) already.

Yes, but no GenericValue(Object/Array) constructors, allowing to directly move an array/object to another value. With such (implicit) constructors (and assignments), wouldn't it be sufficient for all other uses? Adding another bunch of overloads to all value-accepting functions sounds like a lot of boiler-plate to me.

  GenericValue(Object o) : RAPIDJSON_NOEXCEPT : data_(), flags_(kNullFlag) {
    RAPIDJSON_ASSERT(o.ptr_);
    RawAssign(*o.ptr_);
    o.ptr_->SetObject(); // maintain invariant
  }

GenericValue& SetArray() { this->~GenericValue(); new (this) GenericValue(kArrayType); return *this; }

//! Set this value with an array.
GenericValue& SetArray(Array& a) { return *this = *a.ptr_; }
Copy link
Contributor

Choose a reason for hiding this comment

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

After this call, a.ptr_ is null, not an (empty) array. We should add an a.ptr_->SetArray(); here (set for SetObject(Object);` to avoid breaking the invariant.

Secondly, we should check for a.ptr_ not being NULL in an assert, which could happen due to a default constructed Array|Object instance.

miloyip added a commit that referenced this pull request Feb 19, 2016
Templated accessors and range-based for
@miloyip miloyip merged commit 4029ddb into master Feb 19, 2016
@miloyip
Copy link
Collaborator Author

miloyip commented Feb 19, 2016

Thank you @pah

@miloyip miloyip mentioned this pull request Feb 21, 2016
@miloyip miloyip deleted the issue316_templatedaccessors branch June 20, 2017 08:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants