move ctors #267

cdunn2001 · 2015-04-27T21:44:39Z

Add move constructor to Value::CZString
Add unit test for Value move constructor
Allow includer to specify in advance the value for
JSON_HAS_RVALUE_REFERENCES

rebased #264.

cdunn2001 · 2015-04-27T21:53:21Z

We still have to consider what to do for the older branch. We maintain strict binary-compatibility with ancient 0.6.0-rc2 on that branch. Adding a method is fine. I think even adding an rvalue method is fine. So maybe there are no issues.

lanzkron · 2015-04-28T07:16:36Z

Thanks @cdunn2001 !

I see you also fixed my use of nullptr, I only realized it after my PR.

cdunn2001 · 2015-04-28T07:51:24Z

include/json/config.h

 #endif
-#elif defined(__GNUC__) &&  (__GNUC__ > 4 || (__GNUC__ == 4 && __GNUC_MINOR__ >= 5))
-#define JSONCPP_DEPRECATED(message)  __attribute__ ((deprecated(message)))


Oh, I meant to ask: Why was JSON_DEPRECATED dropped for GNUC?

Did I do that? If so it was in error, sorry.

On 28 April 2015 at 10:51, Christopher Dunn [email protected]
wrote:

In include/json/config.h
#267 (comment)
:

#endif
-#elif defined(GNUC) && (GNUC > 4 || (GNUC == 4 && GNUC_MINOR >= 5))
-#define JSONCPP_DEPRECATED(message) attribute ((deprecated(message)))

Oh, I meant to ask: Why was JSON_DEPRECATED dropped for GNUC?

—
Reply to this email directly or view it on GitHub
https://github.com/open-source-parsers/jsoncpp/pull/267/files#r29220618.

cdunn2001 · 2015-04-28T08:54:10Z

Asked about backward-compatibility on stackoverflow.

lanzkron · 2015-04-28T09:09:59Z

Answered on stackoverflow (mostly to get feedback from the community as to whether my understanding is correct).

akrieger · 2015-04-28T23:15:00Z

Can we add move assignment operators as well, so that we can efficiently do

Json::Value root;
root["key"] = functionThatGeneratesJson();

cdunn2001 · 2015-04-29T00:35:47Z

I agree, as in @BillyDonahue comments at #264:

class Value {
    ...
    Value(const Value&);
    Value(Value&&) noexcept;
    Value& operator=(const Value&);
    Value& operator=(Value&&) noexcept;
    void swap(Value& v) noexcept { std::swap(*this, v); }
    friend void swap(Value& a, Value& b) noexcept { a.swap(b); }
};

We also need a macro for noexcept, for the compilers which do not support. Personally, I'm fine with

#if dumb_compiler
#define noexcept
#endif

akrieger · 2015-04-29T01:13:40Z

I like the very political references to 'the compilers which do not support'.

I actually dug into it and it seems the existing operator= implementations exhibit move semantics, as of 45cd949. Looks like that can be reverted to the old copy semantics with the addition of proper move operators. Regardless of the philosophical motivation behind making copy assignment actually be move assignment.

BillyDonahue · 2015-04-29T02:18:51Z

I think the direct #define of a keyword is asking for trouble, especially in a header. Let's be conservative and use JSON_NOEXCEPT. On such platforms, I'd imagine there are plenty of libs that might want to #define noexcept and we'd be conflicting if we did the same.

BillyDonahue · 2015-04-29T02:24:30Z

include/json/config.h

-#define JSONCPP_DEPRECATED(message)  __attribute__ ((deprecated(message)))
-#elif defined(__GNUC__) &&  (__GNUC__ > 3 || (__GNUC__ == 3 && __GNUC_MINOR__ >= 1))
-#define JSONCPP_DEPRECATED(message)  __attribute__((__deprecated__))
+#if defined(_MSC_VER) // MSVC


Can the reindent be its own PR? It's hard to separate cosmetics from substance as-is.
It would be nice to emphasize the substantive changes with concise PRs.

Ok. I'll try re-indent first, and rebase this onto that...

cdunn2001 · 2015-04-29T04:11:27Z

You're probably right; not good in a header. JSON_NOEXCEPT then.

I like the very political references to 'the compilers which do not support'.

Yes! I have strong opinions.

cdunn2001 · 2015-04-29T04:16:32Z

@BillyDonahue, would you pull my code from this branch and add whatever other operators we need? I will take of the JSON_NOEXCEPT macro.

lanzkron · 2015-04-29T07:52:44Z

@akrieger regarding move assignment operator:
Currently the assignment operator uses copy and swap, with the addition of a move constructor it would be able to use move and swap for rvalues which is almost as efficient as writing a special move assignment operator.

If you have a look at Json::Value's copy constructor you'll see that it's rather complex and in my opinion duplicating this logic in a move constructor and assignment operator isn't worth it for the potential improvements.

cdunn2001 · 2015-04-29T13:06:13Z

@lanzkron, Do we need any extra methods, beyond what are already in this pull-request? Do we need the move-ctor for CZString at all? I'm not sure where we are.

BillyDonahue · 2015-04-29T13:20:55Z

@cdunn : I'm still a github noob, but I'm figuring out what you need.
I'm a little confused about what that branch represents.

I'd like to ask about compatibility again. We are not strictly adding new
functions here. My ideal suggestion would be that we'd be REMOVING the
by-value "operator=(Value)" assignment and ADDING the by-cref and by-rref
"operator=(const Value&)" and "operator=(Value&&)" assignments.

Old code expecting to find "operator=(Value)" in libjsoncpp.so wouldn't
find it anymore. I'm not saying this is a problem. I haven't been following
the branching plans or compatibility requirements closely enough. I just
want to make sure this point about the disappearing by-value overload is
understood.

On Wed, Apr 29, 2015 at 12:16 AM, Christopher Dunn <[email protected]

wrote:

@BillyDonahue https://github.com/BillyDonahue, would you pull my code
from this branch https://github.com/cdunn2001/jsoncpp/tree/moveSemantics
and add whatever other operators we need? I will take of the JSON_NOEXCEPT
macro.

—
Reply to this email directly or view it on GitHub
#267 (comment)
.

lanzkron · 2015-04-29T13:37:21Z

@cdunn2001
The move-ctor for CZString is good to have (it also helps when assigning from an rvalue due to the copy and swap assignment operator in CZString).

I'm also new to GitHub, who makes the final call? In my opinion having a move-ctor and keeping the copy/swap assignment operator is good enough. In which case I think we're good to go.

Keep in mind that my implementation of Value(Value&&) depends on swap. If we do decide to implement move ctor/op= from scratch then we'll have to duplicate the logic of the copy constructor (and keep them synced in future changes).

I don't currently have the capacity to do this. If that's what we want then we can dump my changes and wait for someone else to code support for move semantics.

cdunn2001 · 2015-04-29T14:48:49Z

@BillyDonahue, we could remove CZString::operator=(), because the likelihood that anyone uses it directly is miniscule. Technically, we shouldn't change Value::operator=(), but I don't think anyone currently relies on binary-compatibility for 1.0, so it's ok. That method in particular is a welcome change because that is already different in the 0.y.z branch. See 1c4f6a2:

-  Value& operator=(Value other);
+  Value &operator=(const Value &other);

We can remove anything in a future 2.0.0 release. At some point, we should move development to a new alpha branch. That's a discussion for another day.

@lanzkron, there is no final call. We can always add new features. It's better to add less, because it's hard to remove features.

akrieger · 2015-04-29T17:08:04Z

@lanzkron ah, yes, you're right, it is a copy and swap.

lanzkron · 2015-04-29T18:29:35Z

@cdunn2001 I assume you mean replacing CZString's operator= with lvalue and rvalue versions, not remove it completely

thelema · 2015-10-19T20:17:15Z

Is there any chance of this being merged? It looks a little dead. I'm hoping to have move constructors to greatly improve performance of building a very large json object in pieces, and I found this open pull request.

cdunn2001 · 2015-10-20T04:30:11Z

I'm concerned about removing symbols. I don't mind removing them from CZString because nobody who cares about binary compatibility would use it. But I don't think we can remove symbols from Value. I think 1.0 is now used in Debian, e.g.

I've just rebased this. Let's take another look now.

* Add move constructor to Value::CZString * Add unit test for Value move constructor * Allow includer to specify in advance the value for JSON_HAS_RVALUE_REFERENCES

adjustments, based on comments in PR.

cdunn2001 · 2015-10-20T04:53:16Z

Ok, I'd be fine with merging this, if there are no objections. I'll wait a couple days...

BillyDonahue · 2015-10-20T14:56:02Z

Looks good. I do wish we could have the move assignment operations.
Is there an advanced branch for that sort of thing free from the binary compatibility concerns?

cdunn2001 · 2015-10-21T06:13:46Z

Soon. The old branches are stable enough that we can soon consider master as a new pre-release for 2.0.

move ctors

cdunn2001 mentioned this pull request Apr 27, 2015

Add Move constructors #264

Closed

cdunn2001 force-pushed the master branch from d7a9f0c to 28d086e Compare April 28, 2015 01:29

cdunn2001 reviewed Apr 28, 2015
View reviewed changes

cdunn2001 force-pushed the moveSemantics branch 3 times, most recently from b68e7cf to 643cd68 Compare April 28, 2015 08:27

cdunn2001 force-pushed the moveSemantics branch from 643cd68 to e4fb8ce Compare April 28, 2015 16:01

BillyDonahue reviewed Apr 29, 2015
View reviewed changes

cdunn2001 force-pushed the master branch from 4994c77 to beae999 Compare October 10, 2015 21:56

cdunn2001 force-pushed the moveSemantics branch from e4fb8ce to 3d5f5c9 Compare October 20, 2015 04:29

move ctors

2b00891

* Add move constructor to Value::CZString * Add unit test for Value move constructor * Allow includer to specify in advance the value for JSON_HAS_RVALUE_REFERENCES

cdunn2001 force-pushed the moveSemantics branch from 3d5f5c9 to 2b00891 Compare October 20, 2015 04:43

Minor

527965c

adjustments, based on comments in PR.

cdunn2001 added a commit that referenced this pull request Oct 21, 2015

Merge pull request #267 from cdunn2001/moveSemantics

34bdbb5

move ctors

cdunn2001 merged commit 34bdbb5 into open-source-parsers:master Oct 21, 2015

cdunn2001 deleted the moveSemantics branch October 21, 2015 06:14

move ctors #267

move ctors #267

Uh oh!

Conversation

cdunn2001 commented Apr 27, 2015

Uh oh!

cdunn2001 commented Apr 27, 2015

Uh oh!

lanzkron commented Apr 28, 2015

Uh oh!

cdunn2001 Apr 28, 2015

Choose a reason for hiding this comment

Uh oh!

lanzkron Apr 28, 2015

Choose a reason for hiding this comment

Uh oh!

cdunn2001 commented Apr 28, 2015

Uh oh!

lanzkron commented Apr 28, 2015

Uh oh!

akrieger commented Apr 28, 2015

Uh oh!

cdunn2001 commented Apr 29, 2015

Uh oh!

akrieger commented Apr 29, 2015

Uh oh!

BillyDonahue commented Apr 29, 2015

Uh oh!

BillyDonahue Apr 29, 2015

Choose a reason for hiding this comment

Uh oh!

cdunn2001 Oct 20, 2015

Choose a reason for hiding this comment

Uh oh!

cdunn2001 commented Apr 29, 2015

Uh oh!

cdunn2001 commented Apr 29, 2015

Uh oh!

lanzkron commented Apr 29, 2015

Uh oh!

cdunn2001 commented Apr 29, 2015

Uh oh!

BillyDonahue commented Apr 29, 2015

Uh oh!

lanzkron commented Apr 29, 2015

Uh oh!

cdunn2001 commented Apr 29, 2015

Uh oh!

akrieger commented Apr 29, 2015

Uh oh!

lanzkron commented Apr 29, 2015

Uh oh!

thelema commented Oct 19, 2015

Uh oh!

cdunn2001 commented Oct 20, 2015

Uh oh!

cdunn2001 commented Oct 20, 2015

Uh oh!

BillyDonahue commented Oct 20, 2015

Uh oh!

cdunn2001 commented Oct 21, 2015

Uh oh!

Uh oh!