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

Add .natvis for MSVC debug view #844

Merged
merged 5 commits into from
Dec 4, 2017
Merged

Add .natvis for MSVC debug view #844

merged 5 commits into from
Dec 4, 2017

Conversation

TinyTinni
Copy link
Contributor

This PR adds a .natvis improving the debug view of nlohmann::basic_json in the MSVC GUI.
Also, included the ..natvis file into CMake install process.

The supported container types for the template arguments object and array are generic and the container's .natvis will be shown (if declared).

Here is an example view from the readme test (MSVC2017): example_view.png

There is also a name bug in the CMakeLists for tests. I did not created a second issue, since it's just the name-prefix which is missing.
I can create a new pull request if you want or you can fix it on your own. (its the first commit).

@nlohmann nlohmann added the platform: visual studio related to MSVC label Nov 26, 2017
@nlohmann
Copy link
Owner

Just out of curiosity: does the .natvis file must be put into the src directory?

@TinyTinni
Copy link
Contributor Author

No, the .natvis can be everywhere. I.e. the outcome library from ned14 puts it into include dir, GSL from Microsoft puts it into root.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 48d7a32 on TinyTinni:develop into cc937de on nlohmann:develop.

@TinyTinni
Copy link
Contributor Author

TinyTinni commented Nov 26, 2017

Checked the build error. Its only on one Mac machine.
I guess the error comes from the cmake version. On the machine, cmake v3.2.2 is running, where sources are not getting exported:

> Targets with INTERFACE_SOURCES may not be exported with the export() or install(EXPORT) commands. This limitation may be lifted in a future version of CMake.

This restriction is gone with cmake v3.3.

possible ad-hoc solutions:

  • adding cmake version and/or compiler check before adding the sources
  • increase minimum required cmake version (regarding to the copyright, cmake 3.2 was current in 2015)

I don't know what you prefer (or, if nothing, discard the PR).

Edit: CMake Error Message from the machine:

CMake Error: Target "nlohmann_json" has a populated INTERFACE_SOURCES property. This is not currently supported.

and CMake fails. Therefore it is the error mentioned above.

@trilogy-service-ro
Copy link

Can one of the admins verify this patch?

@TinyTinni
Copy link
Contributor Author

added compiler & cmake version check.

For non MSVC-Version, the .natvis file is not needed and I guess it's a good idea no not install it on these system. When I already implemented a condition, I see no problem in adding a version check.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling af775dd on TinyTinni:develop into cc937de on nlohmann:develop.

src/json.natvis Outdated
</Expand>
</Type>

</AutoVisualizer>
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please move this file to the root? I do not want to confuse people that have no idea what a .natvis file is. I may clean up the repo and come up with a misc folder or something, but right now, the root would be fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure,
had to edit the install procedure for it.
Its now in the root dir and will be installed to the root dir /install-prefix dir.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 8e9a879 on TinyTinni:develop into cc937de on nlohmann:develop.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 24fe572 on TinyTinni:develop into cc937de on nlohmann:develop.

Copy link
Owner

@nlohmann nlohmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

@nlohmann nlohmann self-assigned this Dec 4, 2017
@nlohmann nlohmann added this to the Release 3.0.0 milestone Dec 4, 2017
@nlohmann nlohmann merged commit f4c0160 into nlohmann:develop Dec 4, 2017
@nlohmann
Copy link
Owner

nlohmann commented Dec 4, 2017

Thanks a lot!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants