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

Not working for classes containing "_declspec(dllimport)" in their declaration #1108

Closed
shikhartanwar opened this issue May 25, 2018 · 14 comments
Assignees
Milestone

Comments

@shikhartanwar
Copy link

shikhartanwar commented May 25, 2018

Hi,

I have a 3rd party SDK included in my project that has class declarations like below -
Let's assume the class name as Person (simplified the code for reporting purposes) -

class _declspec(dllimport) Person 
{
       public std::string name;
}

I am trying to serialize the class using the below to_json function as mentioned in the documentation -

#include <Person.h>
#include <nlohmann/json.hpp>
using json = nlohmann::json;
void to_json(json& j, Person & o)
{
	j = json{ { "value", o.name } };
}

Person p = new Person();
p.name = "Test";
json j = p;

However even though everything is syntactically correct, I am getting the below errors on building my project -

  1. could not find to_json() method in T's namespace
  2. 'force_msvc_stacktrace': is not a member of 'Person'
  3. 'force_msvc_stacktrace': undeclared identifier
  4. forcing MSVC stacktrace to show which T we're talking about.

I have also tried creating a simple class without the _declspec(dllimport) and that works like a charm.

Can someone please help in this regard?

Details:

  1. Compiler: Visual Studio 2017
  2. Operating System: Windows
@theodelrieu
Copy link
Contributor

Hello,

Assuming your code is not too different from what you reported, my first guess would be that the second argument of to_json must be a const reference.

void to_json(json&, Person const&){}

Did you put the const reference when trying with a different class? That would explain why it worked.

@shikhartanwar
Copy link
Author

shikhartanwar commented May 25, 2018

Nope using a const reference also does not make a difference. When I tried with my own class as compared to the class coming from the 3rd party SDK, I declared the to_json function as below -

void to_json(json& j, Person*  o);

No other definition was working for me. However, when I used the same logic for the class coming from the 3rd party SDK, it started throwing the errors that I mentioned in my previous comment

@gregmarr
Copy link
Contributor

Is the type from the third party in a namespace?

@theodelrieu
Copy link
Contributor

Oh, wait I did miss the part about third party type...

Please take a look here, that should help solve your problem.

@shikhartanwar
Copy link
Author

shikhartanwar commented May 25, 2018

@theodelrieu I am not sure if the specified section helps me. I know for a fact that the 3rd party library does not contain boost::optional or std::filesystem::path namespaces.
Shouldn't the generic serializer work in that case?

@gregmarr Yes!

@theodelrieu
Copy link
Contributor

namespaces boost and std are merely examples in this section. The same reasoning apply to every third party namespace as well.

@shikhartanwar
Copy link
Author

shikhartanwar commented May 25, 2018

I know. But the 3rd party library does not use any custom namespace.

Instead, I am consuming it as a DLL that exposes its symbols using the __declspec(dllimport) specifier.

@gregmarr
Copy link
Contributor

FYI, namespaces and dllexport are not mutually exclusive.

I'm confused. I asked if it uses a namespace, and you said yes to me, but then you said one message later that it doesn't use a namespace. That answer is VERY important. If the type is in a namespace, then the to_json function needs to be in the same namespace.

@shikhartanwar
Copy link
Author

shikhartanwar commented May 25, 2018

@gregmarr Sorry for the confusion but the third party type is not in a namespace. I read the documentation and was aware of the fact that the to_json function needs to be in the same namespace as that of the type. That's why I raised the bug as it is was not working for me.

Also, I am not sure as to how dllexport functions internally and what changes need to be done to make it work with this library because it was not working with the generic serializer.

Please let me know if you need any other details. I am more than willing to help in this matter and I'll make sure I do my due diligence.

@gregmarr
Copy link
Contributor

Person p = new Person();
p.name = "Test";
json j = p;

This code can't be correct, as you are assigning a Person* to a Person, so it shouldn't even compile, and then if you fix that, you're assigning a Person* to json when you should be assigning a Person.

@gregmarr
Copy link
Contributor

Just to make sure that we're looking at the right thing, can you post the exact test code and error message if possible?
If it works without dllimport but fails with it, then there's probably something weird that MSVC is doing.

@shikhartanwar
Copy link
Author

shikhartanwar commented May 26, 2018

Ok give me some time. I'll post similar/simplified code to demonstrate the workflow and the errors that i am encountering.

I would have to make a custom DLL project that exposes the functions in a similar manner, as the third party library that I am talking about has been developed inhouse. So, I can't really post the exact code here.

@nlohmann nlohmann added the state: needs more info the author of the issue needs to provide more details label May 27, 2018
@shikhartanwar
Copy link
Author

Hi guys,

I was able to get this work while I was building the sample application. Basically I was maintaining the to_json methods in a separate .hpp file and that file was not being included wherever the json j = p serializations were written. Because of this, the to_json methods were not discoverable. I was wrong in assuming that the methods would be discoverable in the global namespace.

Maybe we can cover this via documentation and be explicit about it so that no one else runs into this issue?

Anyways, thanks a lot for your prompt responses. Now I will go on and use this awesome library.

@nlohmann nlohmann removed the state: needs more info the author of the issue needs to provide more details label May 28, 2018
@nlohmann
Copy link
Owner

Alright, I shall add a note to the documentation.

@nlohmann nlohmann added this to the Release 3.1.3 milestone May 28, 2018
@nlohmann nlohmann self-assigned this May 28, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants