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

Reuse memory in to_cbor and to_msgpack functions #476

Closed
a-sv opened this issue Feb 27, 2017 · 13 comments
Closed

Reuse memory in to_cbor and to_msgpack functions #476

a-sv opened this issue Feb 27, 2017 · 13 comments
Assignees
Labels
aspect: binary formats BSON, CBOR, MessagePack, UBJSON kind: enhancement/improvement solution: proposed fix a fix for the issue has been proposed and waits for confirmation
Milestone

Comments

@a-sv
Copy link

a-sv commented Feb 27, 2017

It would be good to add the implementation of functions for memory reuse purposes:

static void to_cbor(const basic_json& j, std::vector<uint8_t> &result)
{
    result.clear();
    to_cbor_internal(j, result);
}

static void to_msgpack(const basic_json& j, std::vector<uint8_t> &result)
{
    result.clear();
    to_msgpack_internal(j, result);
}
@nlohmann
Copy link
Owner

I think if such functions are added, the should not clear the vector - it should be the user's responsibility.

@a-sv
Copy link
Author

a-sv commented Feb 27, 2017

Yes, perhaps it would be better.

@tksatware
Copy link

Since we already are at C++11, a return value would be clearer. Move semantics already takes care of passing return values without additional memory handling, also it can be directly used in other functions.

The void return value also forces the break of a call chain like
httprequest::send( json::to_cbor( request ) );
which I prefer.

@a-sv
Copy link
Author

a-sv commented Feb 27, 2017

It is not about copying on assignment. As an example of using such functions - some service exchange messages cbor or msgpack format and using a buffer to receive and transmit them. I do not mind to call these functions like - otherwise, but would like to have them available.

@TurpentineDistillery
Copy link

I think if such functions are added, the should not clear the vector - it should be the user's responsibility.

This is not a universally accepted convention, however. It more depends on the context - what would make most sense. For example, std::getline clears out the string passed to it rather than simply appending.

Usually, to make it unambiguous that the function will not clear out a container, but will rather append to it, the convention is to pass back_insert_iterator rather than the object itself.

@gregmarr
Copy link
Contributor

You could also put _append on the function name.

@nlohmann nlohmann added the aspect: binary formats BSON, CBOR, MessagePack, UBJSON label Mar 28, 2017
@nlohmann
Copy link
Owner

I'm just wondering whether it would make sense to provide a reference to a vector when we do not know the size and hence cannot control whether the vector would use other memory anyways.

@a-sv
Copy link
Author

a-sv commented Apr 11, 2017

This is useful if we use the same vector multiple times. When the "clear" method is executed, the vector does not free up memory and can reuse it.

@nlohmann
Copy link
Owner

nlohmann commented May 8, 2017

Alright, so I would propose a function like

template<typename CharT, typename std::enable_if<
             std::is_integral<CharT>::value and
             sizeof(CharT) == 1, int>::type = 0>
static void to_cbor(const basic_json& j, std::vector<CharT>& vec)

which writes to the vector vec, but does not take care of clearing it before.

As #477 would also introduce a function like

static void to_cbor(const basic_json &j, std::ostream &o)

I would propose to deprecate the

static std::vector<uint8_t> to_cbor(const basic_json& j)

function to unify the function signatures.

What's your opinion on this?

@a-sv
Copy link
Author

a-sv commented May 24, 2017

Looks good. I like.

@nlohmann
Copy link
Owner

Opened #623 as place to discuss the structure of the parsing functions.

nlohmann added a commit that referenced this issue Jul 23, 2017
- You can now pass a reference to a vector to the to_cbor and to_msgpack functions. The output will be written (appended) to the vector. #476

- You can now pass an output stream with uint8_t character type to the to_cbor and to_msgpack functions. #477

- You can now read from uint8_t */size in the to_cbor and to_msgpack functions. An input adapter will be created from this pair, so you need to use braces. #478
@nlohmann nlohmann added the solution: proposed fix a fix for the issue has been proposed and waits for confirmation label Jul 23, 2017
@nlohmann
Copy link
Owner

Function

static void to_cbor(const basic_json& j, detail::output_adapter<uint8_t> o)
{
    binary_writer(o).write_cbor(j);
}

now allows to write CBOR (and similarly, MessagePack) to the following types from which an output_adapter<uint8_t> can be constructed:

  • std::vector< uint8_t >&
  • std::basic_ostream< uint8_t >&
  • std::basic_string< uint8_t >&

Note the function does not clear passed vectors.

@nlohmann nlohmann self-assigned this Jul 30, 2017
@nlohmann nlohmann added this to the Release 3.0.0 milestone Jul 30, 2017
@nlohmann
Copy link
Owner

Done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
aspect: binary formats BSON, CBOR, MessagePack, UBJSON kind: enhancement/improvement solution: proposed fix a fix for the issue has been proposed and waits for confirmation
Projects
None yet
Development

No branches or pull requests

5 participants