Skip to content

templatized output adapters#2173

Closed
FrancoisChabot wants to merge 3 commits intonlohmann:developfrom
FrancoisChabot:2172-templated-output-adapter
Closed

templatized output adapters#2173
FrancoisChabot wants to merge 3 commits intonlohmann:developfrom
FrancoisChabot:2172-templated-output-adapter

Conversation

@FrancoisChabot
Copy link
Contributor

@FrancoisChabot FrancoisChabot commented Jun 5, 2020

This is a draft Fix for #2172.

The output_adapter_protocol is replaced by templatizing the serializer on the output adapter.

The PR also adds the basic_json::dump_to() method, which allows for outputing json to arbitrary user-defined sinks.

Valid sinks are:

  • std::basic_string<>, This will append to the string very efficiently
  • any type that passes std::iterator_traits<T>::iterator_tag == std::output_iterator_tag
  • Any type for which std::back_inserter(T) works. In practice, that means having a push_back(char) function.

If all else fails, the output will be assumed to be:

  • a type that has a ostream-like interface (put(c) and write(ptr, len))

TODO:

  • minor cleanup
  • test cases
  • documentation
  • before and after benchmarks. Done: Benchmarks haven't moved.

Pull request checklist

Read the Contribution Guidelines for detailed information.

  • Changes are described in the pull request, or an existing issue is referenced.
  • The test suite compiles and runs without error.
  • Code coverage is 100%. Test cases can be added by editing the test suite.
  • The source code is amalgamated; that is, after making changes to the sources in the include/nlohmann directory, run make amalgamate to create the single-header file single_include/nlohmann/json.hpp. The whole process is described here.

@FrancoisChabot FrancoisChabot requested a review from nlohmann as a code owner June 5, 2020 20:35
@FrancoisChabot FrancoisChabot marked this pull request as draft June 5, 2020 20:35
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling a3fe169 on FrancoisChabot:2172-templated-output-adapter into 7444c7f on nlohmann:develop.

@FrancoisChabot
Copy link
Contributor Author

A comment in the reddit post about the 3.8.0 release caught my attention:

https://www.reddit.com/r/cpp/comments/h8w7e0/json_for_modern_c_380/fuuozqb?utm_source=share&utm_medium=web2x

That and the dummy hash implementation that allocates (!) probably does more harm than good.

This would actually be easy to fix by having the hash<>::operator() do a dump_to() to an interface that does the hashing as passthrough.

@nlohmann
Copy link
Owner

A comment in the reddit post about the 3.8.0 release caught my attention:

https://www.reddit.com/r/cpp/comments/h8w7e0/json_for_modern_c_380/fuuozqb?utm_source=share&utm_medium=web2x

That and the dummy hash implementation that allocates (!) probably does more harm than good.

This would actually be easy to fix by having the hash<>::operator() do a dump_to() to an interface that does the hashing as passthrough.

I also saw that comment. I think we can also implement a small recursive function that collects and combines the individual hashes. Using the output of dump and hashing the result string was lazy and naive...

@nlohmann
Copy link
Owner

A comment in the reddit post about the 3.8.0 release caught my attention:

https://www.reddit.com/r/cpp/comments/h8w7e0/json_for_modern_c_380/fuuozqb?utm_source=share&utm_medium=web2x

That and the dummy hash implementation that allocates (!) probably does more harm than good.

This would actually be easy to fix by having the hash<>::operator() do a dump_to() to an interface that does the hashing as passthrough.

FYI: I tried to fix this in #2269 - comments welcome!

@nlohmann
Copy link
Owner

@FrancoisChabot Any idea how to proceed here?

@stale
Copy link

stale bot commented Aug 16, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the state: stale the issue has not been updated in a while and will be closed automatically soon unless it is updated label Aug 16, 2020
@stale stale bot closed this Aug 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

state: stale the issue has not been updated in a while and will be closed automatically soon unless it is updated

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants