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

Vs14 fixes #105

Closed
wants to merge 3 commits into from
Closed

Vs14 fixes #105

wants to merge 3 commits into from

Conversation

kepkin
Copy link

@kepkin kepkin commented Jul 15, 2015

Well, that's it. Everything you need to make it compile and fully work under MSVC 2015.
I'm afraid Appveyor won't be able to build it, as it required to much resource to build unit.cpp in release mode.

@gregmarr
Copy link
Contributor

C++11 allows unions to have members with non-trivial constructors. VS2015 RC now supports this according to the feature table at http://blogs.msdn.com/b/vcblog/archive/2015/04/29/c-11-14-17-features-in-vs-2015-rc.aspx

When you use types with non-trivial constructors and destructors in the union, you need to do more management of the union to change the active member, manually calling the destructor on the previous active member and placement new on the new active member.

http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2008/n2544.pdf

@gregmarr
Copy link
Contributor

I know this wasn't first introduced in this commit, but this shouldn't use __basic_json as a typedef. Identifiers that start with two underscores are reserved to the C++ implementation (compiler and standard library). That should be renamed to something else, like basic_json_t.

nlohmann added a commit that referenced this pull request Jul 16, 2015
- additionally, switch off optimization flags to maybe allow build to
complete on AppVeyor
@nlohmann
Copy link
Owner

Hi @kepkin, thanks a lot for the PR!

@nlohmann
Copy link
Owner

Hey @gregmarr, you are right with __basic_json, I changed the name to basic_json_t.

About the union: I applied @kepkin's change to a struct, because I do not understand the details. However, you write

When you use types with non-trivial constructors and destructors in the union, you need to do more management of the union to change the active member, manually calling the destructor on the previous active member and placement new on the new active member.

However, in the code, we never change the active member.

@gregmarr
Copy link
Contributor

If you're fine with it being a struct, that's probably the easiest path. Otherwise you have to be sure to manually construct the objects, and that's probably going to be difficult cross-platform.

@nlohmann
Copy link
Owner

Hi @kepkin again, I have a question with respect to CMake, msbuild, and Appveyor you may help me with: You reported that Appveyor times out when it tries to compile the unit tests. That is very unfortunate but they offer 100% coverage, it would be nice to at least compile them with MSVC to avoid regressions.

In the CMakeLists.txt file, I added -Od to CMAKE_CXX_FLAGS to switch off optimizations. However, these flags seem to be ignored when cmake --build . --config Release is started (see https://ci.appveyor.com/project/nlohmann/json/build/515).

Do you have an idea how to influence this? Or whether there are different ways to make Appveyor compile the code?

@gregmarr
Copy link
Contributor

@nlohmann
Copy link
Owner

@nlohmann nlohmann closed this Jul 16, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants