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

Fix -Weffc++ warnings (GNU 6.3.1) #496

Merged
merged 3 commits into from
Mar 11, 2017
Merged

Fix -Weffc++ warnings (GNU 6.3.1) #496

merged 3 commits into from
Mar 11, 2017

Conversation

TedLyngmo
Copy link
Contributor

Adding constructors and assignment operators where needed to keep -Weffc++ happy.
src/json.hpp regenerated with re2c 0.16 and manually edited to keep the indentation ok.

@@ -6168,6 +6168,21 @@ class basic_json
thousands_sep(!loc->thousands_sep ? '\0' : loc->thousands_sep[0]),
decimal_point(!loc->decimal_point ? '\0' : loc->decimal_point[0])
{}
serializer(const serializer& cpy)
Copy link
Owner

Choose a reason for hiding this comment

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

We don't actually need to copy a serializer, so this should rather be

        serializer(const serializer&) = delete;
        serializer& operator=(const serializer&) = delete;

to make GCC happy without adding unneeded features.

Copy link
Contributor

Choose a reason for hiding this comment

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

I disagree, we should = default everything, since this is the actual behaviour.

Deleting them would potentially break code for no good reason.

I've read some comments about -Weff-c++ not being very relevant, (and I believe this warning is a bit too much).

But since this PR is really short, I think we can still merge it

Copy link
Owner

Choose a reason for hiding this comment

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

The serializer class is private and only used inside the dump function, so no code should rely on it. Doing nothing and relying on default behavior is fine (I could live with the -Weffc++ warnings), but making explicit that we do not intent to copy serializers feels cleaner.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree that there is no need to copy it in the first place, since this is like a traits struct, with no attributes.

I didn't look at the diff, but if we need to add explicit constructors on stuff like detail::is_nested_class, just to support this warning, I really think this isn't worth it, the tests do not build with it, and I believe people who want to use it in their codebase should include json.hpp with -isystem

Copy link
Owner

Choose a reason for hiding this comment

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

I added my proposed two lines to json.hpp and the -Weffc++ warnings disappeared. I think there is no need to change other code

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, but I still think we should = default all operations if we're going down that road.

It's an empty struct after all. Also, nobody ever declares a variable of type std::is_same<int, void> pointless;.

However pointless is still copyable, moveable etc.

This might be nit-picking, but I don't see a good reason to delete those constructors.

Copy link
Owner

Choose a reason for hiding this comment

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

I'm not talking about changing anything but the serializer class.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh wait a Minute, iI thought it was the adl_serializer!

Sorry I'm on my phone , didn't get that!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi! I made the proposed change (and made the new serializer methods private too).
Do I need to create a new pull request for that?

Copy link
Owner

Choose a reason for hiding this comment

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

No, you can change this PR. Could you check why AppVeyor failed?

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.002%) to 99.963% when pulling 9cfb777 on TedLyngmo:fix_effcplusplus_warnings into 758c4ad on nlohmann:develop.

@nlohmann
Copy link
Owner

@TedLyngmo, thanks! I shall merge when Travis finished!

@nlohmann nlohmann added this to the Release 3.0.0 milestone Mar 11, 2017
@nlohmann nlohmann self-assigned this Mar 11, 2017
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.002%) to 99.963% when pulling 70bbf19 on TedLyngmo:fix_effcplusplus_warnings into 758c4ad on nlohmann:develop.

@nlohmann nlohmann merged commit 754ce0b into nlohmann:develop Mar 11, 2017
nlohmann added a commit that referenced this pull request Mar 11, 2017
Ran “make pretty” and added a note to the README file.
@TedLyngmo TedLyngmo deleted the fix_effcplusplus_warnings branch March 24, 2017 11:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants