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

add GenericDocument<>::Swap #376

Merged
merged 4 commits into from
Jul 13, 2015
Merged

Conversation

pah
Copy link
Contributor

@pah pah commented Jul 3, 2015

See #368.

@miloyip
Copy link
Collaborator

miloyip commented Jul 4, 2015

I suggest not to include algorithm for swap. As that header is quite big and may increase compilation overhead. How about creating one internally?

@Kosta-Github
Copy link
Contributor

Why not using lowercase "swap" for the method name? In that case it could work hand in hand with std::swap() and std::swap(doc1, doc2) would automatically pick up the optimized implementation; otherwise std::swap() would do the unoptimized create-copy-temp-from-b, copy-a-to-b, copy-temp-to-a thing...

@pah
Copy link
Contributor Author

pah commented Jul 5, 2015

@miloyip Ok, I'll try to have a look.

@Kosta-Github Providing a lower-case swap member function would break the consistency of the RapidJSON API function naming conventions. Instead, we (or even users) could provide a lower-case swap in the rapidjson namespace, which should achieve the same thing.

@Kosta-Github
Copy link
Contributor

Yeah, I am aware of the inconsistency but still think that easy/efficient interop with the STL is also worth to be considered.

Users most probably will not provide their own rapidjson::swap() function, because the usage of std::swap(doc1, doc2) already works out-of-the-box, but unfortunately with less-than-optimal performance.

This avoids the dependency on the <algorithm> header, as suggested by
@miloyip in Tencent#376.
@pah
Copy link
Contributor Author

pah commented Jul 10, 2015

@miloyip: I have now added a new helper function internal::Swap(), which avoids the dependency on the <algorithm> header.

@Kosta-Github: On my platform, std::swap doesn't work out of the box for GenericDocument/GenericValue as both classes are not copyable by default. If it works on your platform, then probably due to the enabled C++11 move support. In the latter case, the default swap is nearly as efficient than the explicit one.

We could add an "inline friend" function to GenericValue/GenericDocument to enable the common swap idiom:

    // add to GenericValue, GenericDocument (analogously)
    friend inline void swap(GenericValue& a, GenericValue& b) RAPIDJSON_NOEXCEPT
       { a.Swap(b); }

   // enables common pattern:
   using std::swap;
   swap(v1, v2); // calls GenericValue's friend through argument-dependent lookup

@miloyip, what do you think?

@miloyip
Copy link
Collaborator

miloyip commented Jul 12, 2015

@pah I think both changes are OK for me.

@pah
Copy link
Contributor Author

pah commented Jul 13, 2015

I've just added the two free-standing friend swap functions.

miloyip added a commit that referenced this pull request Jul 13, 2015
add GenericDocument<>::Swap with std::swap() support
@miloyip miloyip merged commit 823b731 into Tencent:master Jul 13, 2015
@miloyip
Copy link
Collaborator

miloyip commented Jul 13, 2015

Brilliant~

@pah pah deleted the feature/document-swap branch July 13, 2015 14:40
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.

3 participants