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

The library cannot work properly with custom allocator based containers #766

Closed
imalyavskiy opened this issue Oct 6, 2017 · 17 comments
Closed
Labels
solution: proposed fix a fix for the issue has been proposed and waits for confirmation

Comments

@imalyavskiy
Copy link

imalyavskiy commented Oct 6, 2017

Motivation:
In order to be able to pass std::string std::vector std::list containers between different binaries it was required to define a custom allocator because the project I am involved in uses binaries with statically linked CRT.

Issue:
Once I defined own allocator and std containers that using it I faced a problem - if I need to put data from std::string to my customized std::basic_string I always need to copy data. Suppose tmp::string is my customized string and std::string is usual string. So in order to put data from 2nd to 1st I need to use c_str() each time. The best solution is to instantiate the nlohmann::basic_json with my customized containers. But inside the library its utility classes do not take into account the string_t alias and its string_t::value_type as well thus this leads to errors. I tried to push required types to each class inside the library that use std::string, char, std::vector etc... I did a lots of work but suddenly faced with difficulties that I just cannot understand. So I'm asking a society to help me to solve the issue and improve the json library as a result - to make it truly(or almost truly) template.

Here is the test project that does not compile because static assert fired up with a statement that the "to_json" function with my custom types does not exist. The thing that I am wondering of is that the "to_json" function based on custom types exists(as it follows from the build log) and this confuses me a lot.

The fork of the JSON parser attached as sub module. And this is a MS VS 2017 project.

Thanks a lot.

PS: build.log.txt

@nlohmann
Copy link
Owner

nlohmann commented Oct 6, 2017

Unfortunately, allocators are poorly or not supported at the moment (see #161 or #268). There were some efforts to fix this, but it would mean rewriting significant parts of the library. We may get there if we get to work on #456.

Recently (and somewhat related), there were some workarounds to use a different map for objects, see #546 and #485. They both rely on defining some intermediate types that then specialized basic_json. Maybe this approach could help you.

Apart from this, I definitely agree that custom allocators should be supported. It has been tried at least twice. Nonetheless, maybe someone has an idea how to fix your concrete problem without asking you to wait until we finally rewrite the necessary code to get there.

@nlohmann nlohmann added the state: help needed the issue needs help to proceed label Oct 6, 2017
@imalyavskiy
Copy link
Author

Niels I always rewrote a lot, just check the diff.

@nlohmann
Copy link
Owner

nlohmann commented Oct 6, 2017

As the underlying error is

error C2338: could not find to_json() method in T's namespace

maybe @theodelrieu has an idea. I won't be able to have a look at your fork before Sunday.

@nlohmann
Copy link
Owner

I had a brief look at your code, but I am also not sure how to fix the error.

@imalyavskiy
Copy link
Author

:(

@theodelrieu
Copy link
Contributor

Sorry for the late reply... This should be resolved once allocators are properly supported.

Meanwhile, you can specialize adl_serializer for each of your types:

namespace {
template <>
struct adl_serializer<tmp::string> {
  static void from_json(const json& j, tmp::string& s) {
    // TODO
  }

  static void to_json(json& j, const tmp::string& s) {
    // TODO
  }
};
}

@imalyavskiy
Copy link
Author

It is not clear to me where do I need to specialize it. As I see this specialization must be passed to the basic_json<> template parameter list as the last parameter. But in you sample json already specialized by use of standard types.

@theodelrieu
Copy link
Contributor

You should specialize it inside one of your source files, not inside the library's code.

You don't have to pass a new serializer as the last parameter of basic_json, at least I don't think you need your own serializer in this case.

@imalyavskiy
Copy link
Author

imalyavskiy commented Oct 12, 2017

Hey @theodelrieu. I did as you said and now compiler states other error. See attached log.
build.log.txt

This is because of the

// specialization of std::swap, and std::hash
namespace std
{
    // ....
}

@theodelrieu
Copy link
Contributor

I think I misunderstood what you wanted to do.

I was talking about serializing your custom types from/to JSON, without changing the nlohmann::basic_json template arguments.

@imalyavskiy
Copy link
Author

imalyavskiy commented Oct 12, 2017

No I wanted to completely replace the std::string with my instance of std::basic_string from my custom allocator. Once the std::basic_string specialization can be set into the basic_json thus I supposed that this should work and I've failed. As I understand the meta programming the class interface should stay constant(same as std::basic_string). But once I put specialized std::basic_string the entire basic_json and its utility classes must share this type. But this is not true. I really wanna fix this. I like the similar to std interface of this json lib.

@imalyavskiy
Copy link
Author

Any ideas?

@theodelrieu
Copy link
Contributor

I didn't have much time to take a look, I'll try to make your code work today or this weekend!

@imalyavskiy
Copy link
Author

Glad to hear this!

@theodelrieu
Copy link
Contributor

I've managed to make it compile. However I don't think we should implement proper support for custom allocators before releasing 3.0.

@imalyavskiy I've sent you an email with the patches.

@nlohmann nlohmann added solution: proposed fix a fix for the issue has been proposed and waits for confirmation and removed state: help needed the issue needs help to proceed labels Oct 16, 2017
@nlohmann
Copy link
Owner

Can I close this issue?

@imalyavskiy
Copy link
Author

@theodelrieu, thanks a lot for the time you have spent on this issue. @nlohmann, yes sure at least I see that this issue taken into account and probably will be fixed in version 3.0. Thanks a lot guys!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
solution: proposed fix a fix for the issue has been proposed and waits for confirmation
Projects
None yet
Development

No branches or pull requests

3 participants