Value::swap does not swap some members

[cout]

Looks like it fails to swap itemIsUsed_, memberNameIsStatic_, and comments_. If this is intentional, it deserves a comment.

[cdunn2001]

Feel free to add whatever comment you consider appropriate. The main author of swap and the comment-related features does not have time to work on this project right now. (I avoid all that myself.) I'll leave this issue open for now.

[datadiode]

Found this comment in old code at sourceforge:
/// Swap values.
/// \note Currently, comments are intentionally not swapped, for
/// both logic and efficiency.

[cdunn2001]

@datadiode, that comment is still here. Maybe that just needs to be expanded.

[BillyDonahue]

If it were up to me, I would like the swap to be changed to swap
everything, and not leave the comments behind. The comments are part of the
object's state.

I mean, if I have a std::vector and I do some sort on it with a
series of swaps, I'd want the values to be completely swapped and not leave
irrelevant comments behind. After the sort, the comments will be attached
to the wrong elements. That's very irregular.

On Wed, Jan 7, 2015 at 10:42 AM, Christopher Dunn notifications@github.com
wrote:

@datadiode https://github.com/datadiode, that comment is still here
https://github.com/open-source-parsers/jsoncpp/blob/master/include/json/value.h#L241.
Maybe that just needs to be expanded.

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

[datadiode]

Value::operator=() relies on Value::swap() to not swap the comments, because it is designed to leave comments in place.

The proposed fix is to not implement Value::operator=() in terms of Value::swap(), but in terms of Value::swapPayload(). The latter leaves comments (as well as source offsets) in place, while the former is revised to follow standard semantics.

[BillyGoto]

operator= should not preserve old comments in the destination. It is
extremely surprising if an algorithm relying on swap produces different
results from an algorithm relying on std::swap, which uses operator=.

sounds like a bug factory.

On Sunday, January 18, 2015, datadiode notifications@github.com wrote:

Value::operator=() relies on Value::swap() to not swap the comments,
because it is designed to leave comments in place.

The proposed fix is to not implement Value::operator=() in terms of
Value::swap(), but in terms of Value::swapPayload(). The latter leaves
comments (as well as source offsets) in place, while the former is revised
to follow standard semantics.

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

ǝnɥɐuop ʎllıq

[cdunn2001]

@BillyGoto, You make a good point. But @datadiode has provided swapPayload() to match operator= (pull #118) which should at least make a bug less likely.

[BillyGoto]

swapPayload doesn't really answer my point, though.

It's dangerous to have a value type for which an in-place sort and an
out-of-place sort produce different results.

On Sun, Jan 18, 2015 at 12:37 PM, Christopher Dunn <notifications@github.com

wrote:

@BillyGoto https://github.com/BillyGoto, You make a good point. But
@datadiode https://github.com/datadiode has provided swapPayload() to
match operator= (pull #118
#118) which should
at least make a bug less likely.

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

ǝnɥɐuop ʎllıq

[cdunn2001]

swap() is just a method. I don't think of it as a name with special significance. But we could rename it if you want. What would be a good name for it?

Or it is really just operator=() that bothers you? Personally, I am not a fan of operators in general, so I would have no problem with reverting to its prior behavior. I just want @cout's and @datadiode's suggested functionality to be available somewhere.