Skip to content

Update writer.h #669

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Sep 11, 2017
Merged

Update writer.h #669

merged 2 commits into from
Sep 11, 2017

Conversation

cdunn2001
Copy link
Contributor

Replace #668, which was not properly rebased.

@cdunn2001 cdunn2001 mentioned this pull request Sep 11, 2017
Closed
@lanzkron
Copy link
Contributor

Thanks for picking this up @cdunn2001 !
Please see the comment on my stackoverflow answer regarding improvements to this pull request. As I said, I did it online on github,com so it needs a lot of scrutiny.

@cdunn2001
Copy link
Contributor Author

From SO comment:

Thanks. Note that you forgot a third class StyledStreamWriter. Also, #pragma warning(pop)has to be done at the end of class declaration..else, the warning remains (at least that's what I see with VS 2015)

Ok. I'll try that. But first I'm looking into new TravisCI build failures that have nothing to do with this PR...

@cdunn2001 cdunn2001 force-pushed the avoid-redundant-depreciation-warnings branch from e595a4d to 132840a Compare September 11, 2017 18:44
@cdunn2001 cdunn2001 merged commit 614671d into open-source-parsers:master Sep 11, 2017
@cdunn2001 cdunn2001 deleted the avoid-redundant-depreciation-warnings branch September 11, 2017 19:01
lanzkron
lanzkron reviewed Sep 11, 2017
View reviewed changes
include/json/writer.h
@@ -270,6 +275,8 @@ class JSONCPP_DEPRECATED("Use StreamWriterBuilder instead") JSON_API StyledWrite
* \sa Reader, Value, Value::setComment()
* \deprecated Use StreamWriterBuilder.
*/
#pragma warning(push)
#pragma warning(disable:4996) // Deriving from deprecated class
Copy link
Contributor

@lanzkron lanzkron Sep 11, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I don't actually think that disabling the deprecation warnings on this class is necessary. They were added to StyledWriter and FastWriter since both those classes derived from Writer which is deprecated (and thus considered by VS to be using Writer). StyledStreamWriter is itself deprecated (same as Writer) and doesn't use a deprecated class.

I know I referred to the stackoverflow comment but I didn't examine the class it referred to. Now I think it's in error. (Although I don't think it's harmful either, I can't really tell since I'm not at my dev machine).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok. I'll remove the push/pop for StyledStreamWriter. Thanks for following up.

cdunn2001 added a commit that referenced this pull request Sep 12, 2017
@cdunn2001
No need to suppress warnings for this particular class
586e7a8 
See #669
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lanzkron lanzkron lanzkron left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@cdunn2001 @lanzkron