Skip to content

explicit operator bool() for Json::Value #228

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

Closed
wants to merge 2 commits into from

Conversation

btolfa
Copy link
Contributor

@btolfa btolfa commented Mar 25, 2015

More convenient c++11 way to work with null Json::Value objects then isNull() or operator!
http://en.wikibooks.org/wiki/More_C++_Idioms/Safe_bool#C.2B.2B11

More convenient c++11 way to work with null Json::Value objects then isNull() or operator!
http://en.wikibooks.org/wiki/More_C++_Idioms/Safe_bool#C.2B.2B11
@@ -347,6 +347,23 @@ Json::Value obj_value(Json::objectValue); // {}
/// Return isNull()
bool operator!() const;
Copy link
Contributor

Choose a reason for hiding this comment

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

If you provide explicit operator bool, then you get operator! for free.
The operand of operator! is contextually converted to bool. I wonder if this operator should be removed.

operator! is no longer necessary because of explicit operator bool()
@cdunn2001
Copy link
Contributor

This article explains the problems with operator bool(). In general, I oppose cast operators because of the confusion they cause. But what's wrong with this?

    if (!!val) {...

Really, operator!() is a mistake too. What should we return when the value is integer 0? Or actually false? isNull() is unambiguous. But operator!() is already part of the API. So just use it ... er ... twice.

@cdunn2001 cdunn2001 closed this Mar 31, 2015
@BillyDonahue
Copy link
Contributor

Chris, are you considering specifically the request for explicit operator bool? That article is from 2004 and predates the explicit operator bool in C++11. The language has been changed deep-down to make hacks like the double-bang and safe_bool class tricks obsolete.

http://stackoverflow.com/questions/6242768/is-the-safe-bool-idiom-obsolete-in-c11

@BillyDonahue
Copy link
Contributor

I think we could define the explicit operator bool to do exactly what operator! does, but with the sign flipped. It currently returns isNull which seems fine to me.

There are no implicit conversions defined for Json::Value, so we're safe with the IsNull interpretation of the explicit operator bool, I'd think. I mean the current situation is this:

Json::Value v(false);
bool b = v;  // doesn't compile.
int i = v;  // doesn't compile either.
// v is considered 'true' because it's holding a value. ANY value.

I think it's reasonable to follow the model of std::optional:
http://en.cppreference.com/w/cpp/experimental/optional/operator_bool

constexpr explicit operator bool() const;
Checks whether *this is in engaged state, i.e. whether the contained value is initialized.

std::optional requires dereferencing syntax to get at its value, as does Json::Value.
While std::optional uses operator* and operator->, Json::Value uses asX() accessors, but conceptually they're similar.

@BillyDonahue BillyDonahue reopened this Mar 31, 2015
@cdunn2001
Copy link
Contributor

Yes, explicit solves some problems, but it is not available in older versions of C++. I don't mind extra functionality in the 1.*.* branch, but this could become confusing. We would have operator!() in 0.*.*, but in 1.*.* that operator would work indirectly though it would not exist (as you pointed out in another comment).

operator!() and !! is a trivial solution. I don't understand why that simple idiom is a problem when using a language that has so many awful warts.

And really, I wish that operator!() didn't exist either. It's not very logical. (Pardon the pun.) isNull() is so much easier to understand.

@BillyDonahue, is this a big deal to you? If so, just push it. I'll add a revert for the 0.*.* branch. I think casting operators are crutches. (I am full of puns today.) But I don't mind. snprintf is what's driving me crazy. This is really a small thing to me.

@BillyDonahue
Copy link
Contributor

I really want to be very clear that I don't think Json should bother with safebool hacks, and that explicit operator bool in C++11 is the only thing we should be supporting here.

operator!() and !! is a trivial solution. I don't understand why that simple idiom is a problem when using a language that has so many awful warts.

Here's an example of where you can't use !!:

if (Json::Value v = ProduceValue(input)) {
  PrintSomehow(v);
} else {
  continue;
}

And another:

while (Json::Value v = ReadNextJsonValue(input)) {
  PrintSomehow(v);
}

It's a bit of an impedance mismatch against modern C++, so I think we should just support the more modern style. Re "awful warts" I don't get it. I mean sure other C++ problems exist, but let's try to solve them one at a time.

Sure there's a workaround, but really if something has a ! it's a little surprise when it isn't usable in an explicit bool context. And little surprises add up.

If we look at the landscape, we have a few type-erased objects in C++ these days as models. There's std::optional, std::function, all of the smart pointers, etc. They're doing this explicit operator bool, and you don't have use it if you don't want to.

Thanks for being flexible, I do happen to think this is a step in the right direction. We can establish a notion that a Json::Value has a truth value indicating its engaged state.

@cdunn2001
Copy link
Contributor

Ok. Good examples and a convincing argument. Let's do it. But note that we will fully require C++11 for that branch. That's fine with me, and we can get serious about adding a move ctor, etc.

* std::cin >> root;
*
* if (auto tag = root["tag"]) {
* // Behavior if tag object with tag key exist
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment is probably overdoing it.
It's also got a grammatical problem and I don't like involving the behavior of operator[] into the explanation. Let's explain it without referring to other API points if we can.

@okodron
Copy link
Contributor

okodron commented Jan 12, 2018

Already merged as #695

@cdunn2001 cdunn2001 closed this Jan 20, 2018
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.

4 participants