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

Fixes Cppcheck warnings #1759

Open
Xav83 opened this issue Sep 19, 2019 · 5 comments

Comments

@Xav83
Copy link
Contributor

@Xav83 Xav83 commented Sep 19, 2019

Hi @nlohmann and other mainteners,

First of all, I must say that I love your work on this librairy.
It feels really easy to use, and to read code using this library ! ❤️

I have created this issue to talk about the warnings that cppcheck found on this project.
I'm currently experimenting with both this library and cppcheck on some little side projects. And doing so, I've ended up with cppcheck warnings me about some elements inside the json library.

So the purpose of this issue is to talk about the warnings from cppcheck, to address them when they are relevant, so that other people which are going to have both this library and cppcheck] in their project, as little as warnings as possible.

Here is the list of the warnings I've got for now:

(1)[include\nlohmann\json.hpp:5076]: (style) Variable 'm_value.object->emplace' is reassigned a value before the old one has been used.
(2)[include\nlohmann\detail\conversions\to_chars.hpp:585]: (style) The expression 'kAlpha >= -60' is always true.
(3)[include\nlohmann\detail\conversions\to_chars.hpp:586]: (style) The expression 'kGamma <= -32' is always true.
(4)[include\nlohmann\detail\output\serializer.hpp:726]: (style) The expression 'thousands_sep != '\0'' is always false because 'thousands_sep' and ''\0'' represent the same value.
(5)[include\nlohmann\detail\output\serializer.hpp:736]: (style) The expression 'decimal_point != '\0'' is always false because 'decimal_point' and ''\0'' represent the same value.
(6)[include\nlohmann\detail\input\binary_reader.hpp:650]: (style) Unsigned expression 'mant' can't be negative so it is unnecessary to test it.
(7)[include\nlohmann\json.hpp:837]: (style) Local variable object shadows outer function
(8)[include\nlohmann\detail\input\input_adapters.hpp:339]: (style) Class 'input_adapter' has a constructor with 1 argument that is not explicit.
(9)[include\nlohmann\detail\input\input_adapters.hpp:342]: (style) Class 'input_adapter' has a constructor with 1 argument that is not explicit.
(10)[include\nlohmann\detail\input\input_adapters.hpp:346]: (style) Class 'input_adapter' has a constructor with 1 argument that is not explicit.
(11)[include\nlohmann\detail\input\input_adapters.hpp:349]: (style) Class 'input_adapter' has a constructor with 1 argument that is not explicit.
(12)[include\nlohmann\detail\input\input_adapters.hpp:352]: (style) Class 'input_adapter' has a constructor with 1 argument that is not explicit.
(13)[include\nlohmann\detail\input\input_adapters.hpp:355]: (style) Class 'input_adapter' has a constructor with 1 argument that is not explicit.
(14)[include\nlohmann\detail\input\input_adapters.hpp:377]: (style) Class 'input_adapter' has a constructor with 1 argument that is not explicit.
(15)[include\nlohmann\detail\input\input_adapters.hpp:421]: (style) Class 'input_adapter' has a constructor with 1 argument that is not explicit.
(16)[include\nlohmann\detail\input\input_adapters.hpp:429]: (style) Class 'input_adapter' has a constructor with 1 argument that is not explicit.
(17)[include\nlohmann\detail\json_ref.hpp:18]: (style) Class 'json_ref' has a constructor with 1 argument that is not explicit.
(18)[include\nlohmann\detail\json_ref.hpp:22]: (style) Class 'json_ref' has a constructor with 1 argument that is not explicit.
(19)[include\nlohmann\detail\json_ref.hpp:26]: (style) Class 'json_ref' has a constructor with 1 argument that is not explicit.
(20)[include\nlohmann\detail\json_ref.hpp:33]: (style) Class 'json_ref' has a constructor with 1 argument that is not explicit.
(21)[include\nlohmann\detail\output\output_adapters.hpp:105]: (style) Class 'output_adapter' has a constructor with 1 argument that is not explicit.
(22)[include\nlohmann\detail\output\output_adapters.hpp:108]: (style) Class 'output_adapter' has a constructor with 1 argument that is not explicit.
(23)[include\nlohmann\detail\output\output_adapters.hpp:111]: (style) Class 'output_adapter' has a constructor with 1 argument that is not explicit.
(24)[include\nlohmann\json.hpp:1162]: (style) Class 'basic_json' has a constructor with 1 argument that is not explicit.
(25)[include\nlohmann\json.hpp:1186]: (style) Class 'basic_json' has a constructor with 1 argument that is not explicit.
(26)[include\nlohmann\json.hpp:1253]: (style) Class 'basic_json' has a constructor with 1 argument that is not explicit.
(27)[include\nlohmann\json.hpp:1290]: (style) Class 'basic_json' has a constructor with 1 argument that is not explicit.
(28)[include\nlohmann\json.hpp:1730]: (style) Class 'basic_json' has a constructor with 1 argument that is not explicit.
(29)[include\nlohmann\detail\output\binary_writer.hpp:869]: (style) Consider using std::accumulate algorithm instead of a raw loop.

Some of them may be false positive, but others can be relevant, so this is why I've made this issue. The good news, is that all the warnings I have got are flagged as "style", not as error or warning, so their are less likely to bother most of people using cppcheck. 😄

If we successly correct all the warnings (and silence the false positive), we may even imagine adding cppcheck to the CI of the library. 🤞

Let me know what you think about this idea. If you agree with the purpose of this issue, I'll gladly help to reach it.

For technical information, I've used the version 1.87 of cppcheck on the version 3.7.0 of the json library, on Windows, with Visual Studio 2017. You can find the repository on which I'm currently running this here

In a few minutes, I will also add a Pull Request which handles the warnings 6 and 29. 🔜

@Xav83

This comment has been minimized.

Copy link
Contributor Author

@Xav83 Xav83 commented Sep 19, 2019

PS: I don't know is this issue should be flagged as a feature request or as anything else, so I made it a regular issue.

@Xav83 Xav83 referenced this issue Sep 19, 2019
4 of 4 tasks complete
@t-b

This comment has been minimized.

Copy link
Contributor

@t-b t-b commented Oct 7, 2019

@Xav83 Closed by #1760?

@Xav83

This comment has been minimized.

Copy link
Contributor Author

@Xav83 Xav83 commented Oct 12, 2019

@t-b I don't think this issue should be closed, for now.
The Pull Request #1760 only fixed the warning 6 and 29.
The others remains unfixed yet

@nlohmann

This comment has been minimized.

Copy link
Owner

@nlohmann nlohmann commented Oct 17, 2019

If I remove the warnings on non-explicit constructors (which are not a bug, but a feature for this library), we are left with the following issues:

(1)[include\nlohmann\json.hpp:5076]: (style) Variable 'm_value.object->emplace' is reassigned a value before the old one has been used.
(2)[include\nlohmann\detail\conversions\to_chars.hpp:585]: (style) The expression 'kAlpha >= -60' is always true.
(3)[include\nlohmann\detail\conversions\to_chars.hpp:586]: (style) The expression 'kGamma <= -32' is always true.
(4)[include\nlohmann\detail\output\serializer.hpp:726]: (style) The expression 'thousands_sep != '\0'' is always false because 'thousands_sep' and ''\0'' represent the same value.
(5)[include\nlohmann\detail\output\serializer.hpp:736]: (style) The expression 'decimal_point != '\0'' is always false because 'decimal_point' and ''\0'' represent the same value.
(7)[include\nlohmann\json.hpp:837]: (style) Local variable object shadows outer function

PRs welcome!

@t-b

This comment has been minimized.

Copy link
Contributor

@t-b t-b commented Oct 23, 2019

And even nicer than a PR fixes these issues would be adding the cppcheck to CI so that the codebase stays clean as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.