Make copy and reference clear at ValueFactory#324
Conversation
ValueFactory .newBinary, .newString, .newArray and .newMap take a mutable argument. Some of them used them as a reference and some of them copied them. This pull-request makes it clear that they copy the argument by default and try to omit the copy if an extra argument is set to true. This change also includes: * newMap(Value... kvs) method which was previously newMap(Value[] kvs) * MessageUnpacker.unpackValue removes a duplicated copy of new byte[] (string and binary) or Value[] (array and map).
|
@xerial Another possible design is adding new methods (such as newBinaryRef, newArrayRef) instead of adding an extra argument. With this way, users will expect that -Ref methods will always use reference. But in fact, they use reference only if |
There was a problem hiding this comment.
The meaning of ref is a little bit ambiguous. How about using boolean createCopy?
There was a problem hiding this comment.
The "correct" name is useReferenceIfPossible but it seems too long. How about boolean omitCopy instead?
I think inverting true/false is confusing because using true as the default seems unexpected.
There was a problem hiding this comment.
I changed it to omitCopy.
|
ValueFactory is just a collection of helper methods and users simply need to use newXXX methods without |
Make copy and reference clear at ValueFactory
ValueFactory .newBinary, .newString, .newArray and .newMap take a
mutable argument. Some of them used them as a reference and some of them
copied them. This pull-request makes it clear that they copy the
argument by default and try to omit the copy if an extra argument is
set to true.
This change also includes: