Skip to content

Allow Json::Value to be used in a boolean context #695

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 3 commits into from
Dec 5, 2017
Merged

Allow Json::Value to be used in a boolean context #695

merged 3 commits into from
Dec 5, 2017

Conversation

wolframroesler
Copy link
Contributor

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.
@wolframroesler
Copy link
Contributor Author

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

@BillyGoto
Copy link

BillyGoto commented Nov 13, 2017 via email

@cdunn2001
Copy link
Contributor

@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.

@cdunn2001 cdunn2001 self-assigned this Nov 16, 2017
@BillyGoto
Copy link

BillyGoto commented Nov 16, 2017 via email

It's no longer needed since we now have operator bool which gives
us operator! for free.
@wolframroesler
Copy link
Contributor Author

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

@wolframroesler
Copy link
Contributor Author

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!

@cdunn2001
Copy link
Contributor

how about merging it?

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

@wolframroesler
Copy link
Contributor Author

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`.
@cdunn2001 cdunn2001 merged commit 9079422 into open-source-parsers:master Dec 5, 2017
@cdunn2001
Copy link
Contributor

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

@wolframroesler wolframroesler deleted the operatorbool branch December 6, 2017 07:49
@cdunn2001
Copy link
Contributor

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;
                            ^

@wolframroesler
Copy link
Contributor Author

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 cdunn2001 mentioned this pull request Dec 21, 2017
cdunn2001 added a commit that referenced this pull request Dec 21, 2017

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
We set this is the Meson build to eliminate warnings, but
c++0x should still work, at least for now.

See #695 for discussion.
@Dani-Hub
Copy link
Contributor

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.

@wolframroesler
Copy link
Contributor Author

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?

@Dani-Hub
Copy link
Contributor

OK, will do so.

@Dani-Hub
Copy link
Contributor

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

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.

None yet

5 participants