Allow Json::Value to be used in a boolean context by wolframroesler · Pull Request #695 · open-source-parsers/jsoncpp (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation15 Commits3 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

In a boolean context, a Value is false if it is null, and true if it is not null. This was already implemented partially by operator! and is now supported fully by adding implicit conversion of Value to bool (through operator bool).

For example:

Json::Value v; ...

// We already had this before (through operator!): if (!v) { // Executed if v is null }

// This commit adds the following (through the new operator bool): if (v) { // Executed if v is not null }

Includes test coverage.

In a boolean context, a Value is "false" if it is null, and "true" if it is not null. This was already implemented partially by operator! and is now supported fully by adding implicit conversion of Value to bool (through "operator bool").

For example:

Json::Value v;
...

// We already had this before (through operator!):
if (!v) {
    // Executed if v is null
}

// This commit adds the following (through the new operator bool):
if (v) {
    // Executed if v is not null
}

Includes test coverage.

Hi, any comments from the maintainers about this change? Did you decide whether or not to merge it yet?

Makes sense to me. Also, if you have an operator bool you get operator! for free. So we don’t need it anymore except for ABI compatibility I guess.

-- ǝnɥɐuop ʎllıq

@BillyGoto, my instinct is to require a bool conversion to be explicit, but I will defer to your judgement.

@wolframroesler, please also removeoperator!() per Billy's suggestion. Sorry for delay in response.

It's no longer needed since we now have operator bool which gives us operator! for free.

Thanks for your input. Removed operator! (the PR now essentially replaces operator! with operator bool).

Hi maintainers,

how about merging it? I dearly need it :) but I don't want to change it in my copy of the repository if it's not going into the official distribution. Thanks!

how about merging it?

@wolframroesler, two of us want operator bool() to be marked explicit. What's your argument against?

Sorry, completely missed those messages. Making the operator explicit is of course the only reasonable way to do it. Will submit another pull request.

We want to do things like:

Json::Value v;
...
if (v) ...;
if (!v) ...;

but not

bool b = v;

so the Value-to-bool conversion operator has to be marked explicit.

We'll have to bump the soversion before we tag a new release. Thanks for the changes.

I just noticed this in a TravisCI build:

../include/json/value.h:404:28: warning: explicit conversion operators only available with -std=c++11 or -std=gnu++11 explicit operator bool() const; ^

jsoncpp 1.* is for C++ 11, isn't it? Is the C++ 11 compiler option set in the Travis build? If it is, is the warning of any significance? If it isn't, can it be disabled via a compiler option?

cdunn2001 added a commit to cdunn2001/jsoncpp that referenced this pull request

Dec 21, 2017

We set this is the Meson build to eliminate warnings, but c++0x should still work, at least for now.

See open-source-parsers#695 for discussion.

cdunn2001 added a commit that referenced this pull request

Dec 21, 2017

We set this is the Meson build to eliminate warnings, but c++0x should still work, at least for now.

See #695 for discussion.

This commit has broken the support for Visual Studio 2012, which doesn't support explicit as keyword for conversion functions. Given that config.h distinguishes C++11 from other versions, I don't understand why during this commit there was not introduced something like JSONCPP_EXPLICIT_OP which is defined empty for the else branch (lines 84-96) and to prefix the added operator with that macro to ensure that support.

Very sorry to hear that. Since I don't have Visual Studio, would you mind submitting a pull request with the code modified as you suggest?

Issue #731 has been created to cope with that problem.