Skip to content

Use macro for override #449

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

Merged
merged 1 commit into from
Mar 22, 2016

Conversation

cdunn2001
Copy link
Contributor

b/c MS VS2010 is supposed to be C++11 but does not fulfull
the entire standard.

Resolves #410.
Re: #430.

b/c MS VS2010 is supposed to be C++11 but does not fulfull
the entire standard.

Resolves open-source-parsers#410.
Re: open-source-parsers#430.
@ya1gaurav
Copy link
Contributor

+#if defined(_MSC_VER) && _MSC_VER <= 1600 // MSVC <= 2010
+# define JSONCPP_OVERRIDE

This is wrong, It should be marked as virtual. Otherwise we are changing the behaviour of class.
Should be like below:

    #if defined(_MSC_VER) && _MSC_VER <= 1600 // MSVC <= 2010
    # define JSONCPP_OVERRIDE virtual

But, still virtual keyword is placed before function name.

@cdunn2001
Copy link
Contributor Author

Are you sure it's wrong? If the base-class function is virtual, then so is the derived, whether we say so or not. And override is used only on the derived.

This was @BillyDonahue's suggestion, and I think it makes sense. Have you seen it actually fail somewhere?

cdunn2001 added a commit that referenced this pull request Mar 22, 2016
@cdunn2001 cdunn2001 merged commit b803b92 into open-source-parsers:master Mar 22, 2016
@cdunn2001 cdunn2001 deleted the override-keyword branch March 22, 2016 02:16
@ya1gaurav
Copy link
Contributor

I agree with your explanation, I think I made some wrong assumption. Thanks for clarification.

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.

2 participants