[boost] [variant] Opinion on boost::safe_get<> and default boost::get<> behavior

[Antony Polukhin]

Hi all,

I'm in doubt and would like to hear opinions on the following issue.

boost::get for boost::variant was not making any compile time checks,
resulting in runtime errors where a compile time error must be:

boost::variant<int, string> v(100);
boost::get<bool>(&v); // returns NULL instead of compile time error
boost::get<bool>(v); // throws exception on runtime instead of compile
time error


Now we have an implementation of compile time checked get in a separate
branch at git:

boost::variant<int, string> v(100);

boost::safe_get<bool>(&v); // compile time error
boost::safe_get<bool>(v); // compile time error
boost::unsafe_get<bool>(&v); // returns NULL instead of compile time error
boost::unsafe_get<bool>(v); // throws exception on runtime instead of
compile time error


I'm in doubt, what the default behavior of boost::get must be:
* safe_get behavior is much closer to Standard Library behavior (just like
std::get for tuples) and allows to avoid errors in user code
* unsafe_get behavior is same as behavior of old boost::get and won't break
user's code if boost::get is used in some generic contexs

What's your opinion?

--
Best regards,
Antony Polukhin

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost

[Peter Dimov]

Antony Polukhin wrote:
...

> boost::variant<int, string> v(100);
>
> boost::safe_get<bool>(&v); // compile time error
> boost::safe_get<bool>(v); // compile time error
> boost::unsafe_get<bool>(&v); // returns NULL instead of compile time error
> boost::unsafe_get<bool>(v); // throws exception on runtime instead of
> compile time error
>
>
> I'm in doubt, what the default behavior of boost::get must be:
> * safe_get behavior is much closer to Standard Library behavior (just like
> std::get for tuples) and allows to avoid errors in user code
> * unsafe_get behavior is same as behavior of old boost::get and won't
> break user's code if boost::get is used in some generic contexs
>
> What's your opinion?

My opinion is that these functions are misnamed. unsafe_get is not unsafe.
I'd say that they are strict and relaxed, respectively, not safe and unsafe.

Regarding the default, there's only one way to find out. Namely, you switch
boost::get to be strict, then see if there are any complaints. (Having the
default be relaxed doesn't seem to make much sense.)

There's nothing wrong (or unsafe) with the old behavior - it consistently
answers the question "does v contain a T" - so the decision can't be made
based on principles alone and must be informed by practice. That is, do the
benefits of the strict get - which are pretty much only catching your typo
when you say get<T>(v) instead of get<T>(w) - outweigh the loss of
functionality.

[Matt Calabrese]

On Tue, Dec 9, 2014 at 9:39 AM, Peter Dimov <li...@pdimov.com> wrote:

> There's nothing wrong (or unsafe) with the old behavior - it consistently
> answers the question "does v contain a T" - so the decision can't be made
> based on principles alone and must be informed by practice. That is, do the
> benefits of the strict get - which are pretty much only catching your typo
> when you say get<T>(v) instead of get<T>(w) - outweigh the loss of
> functionality.

While that's true, the old behavior seems about as compelling to me as if
the language allowed you to static_cast between any two types, but yielded
a null pointer at run-time if there were no valid conversion. Even if it
breaks a bunch of code it's probably better in the long run, unless there
are interesting idiomatic use-cases.

--
-Matt Calabrese

[Vicente J. Botet Escriba]

Le 09/12/14 18:39, Peter Dimov a écrit :

I agree with peter strict/relaxed covers better the
Even if changing the default behavior to strict_get could break some
user code, this will result on a compile error, which wouldn't be a
silent break. If you provide both strict/relaxed versions, the user
could always change its defaulted get<bool>(v) to relaxed_get(bool)(v).

Best,
Vicente

P.S. I don't think there will be too much uses of relaxed_get, but who
know what C++ programmers use ;-)

[Peter Dimov]

Matt Calabrese wrote:
> While that's true, the old behavior seems about as compelling to me as if
> the language allowed you to static_cast between any two types, but yielded
> a null pointer at run-time if there were no valid conversion.

That's not true. Both get variants can fail (at runtime) if the variant
doesn't contain the target, so your code that uses either MUST be prepared
to deal with this case. The difference is only that one of them allows you
to ask "does this variant<X,Y> contain a Z?" (answering "no"), while the
other does not. But for the question "does this variant<X,Y> contain a X?",
in both cases, "no" is still a valid answer. If your code doesn't expect
"no" as an answer, it's broken, in either case.

[Gottlob Frege]

On Tue, Dec 9, 2014 at 1:31 PM, Peter Dimov <li...@pdimov.com> wrote:
> Matt Calabrese wrote:
>>
>> While that's true, the old behavior seems about as compelling to me as if
>> the language allowed you to static_cast between any two types, but yielded a
>> null pointer at run-time if there were no valid conversion.
>
>
> That's not true. Both get variants can fail (at runtime) if the variant
> doesn't contain the target, so your code that uses either MUST be prepared
> to deal with this case. The difference is only that one of them allows you
> to ask "does this variant<X,Y> contain a Z?" (answering "no"), while the
> other does not. But for the question "does this variant<X,Y> contain a X?",
> in both cases, "no" is still a valid answer. If your code doesn't expect
> "no" as an answer, it's broken, in either case.
>

My gut feeling is that "does this variant<X, Y> contain a Z" should be
a compile time error, because it obviously doesn't contain a Z. (Of
course less obvious when variant<X,Y> is spelled 'T'.) And because it
*can* be a compile time error. In most C++ code, you need to go out
of your way (eg dynamic_cast) to do type-stuff at runtime. Typically
it is compile time.

Beyond the strict/relaxed, is there a (compile time) way to ask "could
this variant *ever* contain a Z" ?

Broadly speaking: I'd like Boost to be brave and make breaking changes
like this. The next step after boost:: is std::, and I'd like more
trials and attempts at the boost level, because it is really hard to
change once in std. Currently what I sometimes here in committee
meetings is "but it has been this way in boost for years". But if
boost is afraid to make changes, and the committee is afraid to change
what comes from boost, then it means that std is just taking in
whatever made it past the initial boost review (which is a good review
process, but isn't meant to be the only gate to pass into std).

Tony

[Peter Dimov]

Gottlob Frege wrote:
> My gut feeling is that "does this variant<X, Y> contain a Z" should be a
> compile time error, because it obviously doesn't contain a Z. (Of course
> less obvious when variant<X,Y> is spelled 'T'.)

Or when it's spelled variant<T...>.

> And because it *can* be a compile time error.

Well yes, that is why we are having this discussion.

> In most C++ code, you need to go out of your way (eg dynamic_cast) to do
> type-stuff at runtime.

get<> ALWAYS does type-stuff at runtime. It's not going to get any less
runtimey. It's just going to catch a few (not very common) errors. It's
still a dynamic_cast; the equivalent of the strict get<> would be a
dynamic_cast that tells you, at compile time, "this cast will always fail,
don't do it." Which, in dynamic_cast's case, is - I suspect - going to be a
mixed blessing unless it's a warning, not an error.

That said, my recommendation still stands. Make get<> strict, see what
happens.

[Matt Calabrese]

On Dec 9, 2014 10:33 AM, "Peter Dimov" <li...@pdimov.com> wrote:
> That's not true. Both get variants can fail (at runtime) if the variant
doesn't contain the target, so your code that uses either MUST be prepared
to deal with this case. The difference is only that one of them allows you
to ask "does this variant<X,Y> contain a Z?" (answering "no"), while the
other does not.

Right, but again, this is analogous to something like static_cast allowing
you to cast between unrelated types and not telling you until runtime.
Perhaps I should have related it to dynamic_cast if that makes things more
clear. You need some functionality at run-time, of course, but I don't know
if you gain anything tangible by allowing the code to compile for unrelated
types for which the conversion is known at compile-time to never be
possible. It's hard to say without use-cases, though. I use variants a lot
and I don't see this as desirable, but I may have just not encountered a
use. If I have an integer long in a variant and I accidentally try to grab
an int, I'd certainly like to know at compile-time that the mistake was
made.

[Rob Stewart]

On December 9, 2014 2:44:32 PM EST, Peter Dimov <li...@pdimov.com> wrote:
>
>my recommendation still stands. Make get<> strict, see what
> happens.

+1

___
Rob

(Sent from my portable computation engine)

[Antony Polukhin]

Thank you all for your comments!

strict_get and relaxed_get sound much better that safe_get/unsafe_get, so I
renamed the new methods.
get<> behavior by default would be strict, while old behavior could be
restored by defining BOOST_VARIANT_USE_RELAXED_GET_BY_DEFAULT.

--
Best regards,
Antony Polukhin

[David Stone]

I see no reason to allow code that is always a bug.

There are two possibilities here:

1) The user is expecting the object of that type to be there, but used the
wrong type. This is the example Matt Calabrese gave: the variant contains
long but I accidentally used int. This is a bug.

2) The user is wondering whether the type is there and is trying to get it
if it is. Perhaps they are checking multiple types in sequence. However,
this is a performance bug if the compiler cannot remove the "never going to
work" code. Even if the compiler can remove the code, people reading it
might not be able to do so mentally. Now you have code that will never
execute, but all readers of your code assume it will (or why would you have
written it)? I have worked on code bases with lots of code like this, and I
have never once appreciated the misleading rabbit holes this sends me down
during debugging.

[Olaf van der Spek]

On Wed, Dec 10, 2014 at 3:57 PM, David Stone <da...@doublewise.net> wrote:
> I see no reason to allow code that is always a bug.
>
> There are two possibilities here:
>
> 1) The user is expecting the object of that type to be there, but used the
> wrong type. This is the example Matt Calabrese gave: the variant contains
> long but I accidentally used int. This is a bug.
>
> 2) The user is wondering whether the type is there and is trying to get it
> if it is. Perhaps they are checking multiple types in sequence. However,
> this is a performance bug if the compiler cannot remove the "never going to
> work" code. Even if the compiler can remove the code, people reading it
> might not be able to do so mentally. Now you have code that will never
> execute, but all readers of your code assume it will (or why would you have
> written it)? I have worked on code bases with lots of code like this, and I
> have never once appreciated the misleading rabbit holes this sends me down
> during debugging.

Compilers don't disallow if (0) and I think this construct could occur
in (generic) code. Could the same be true for the variant case?
I.e., is it always a bug?


--
Olaf

[Nevin Liber]

On 10 December 2014 at 17:04, Olaf van der Spek <m...@vdspek.org> wrote:

> Compilers don't disallow if (0) and I think this construct could occur
> in (generic) code. Could the same be true for the variant case?
> I.e., is it always a bug?
>

We'll find out if we harden up get (which I also +1, BTW). :-)

If someone really intends to take advantage of this, they can always
rewrite their code to first check the type list. I suspect the number of
people who deliberately use this feature to be indistinguishable from zero.
:-)
--
Nevin ":-)" Liber <mailto:ne...@eviloverlord.com> (847) 691-1404

[gast128]

> Best,
> Vicente
>
> P.S. I don't think there will be too much uses of relaxed_get, but who
> know what C++ programmers use

Yes there are. The boost::get function is used by us to check if some data
source exist at runtime. It is somewhat like this, where a data source can
be in single datum mode or be buffered:

typedef boost::variant<int*, double*, std::vector<int>*> DataSource;

template <typename T>
class Sink
{
bool Connect(const DataSource& crDataSource)
{
//check availability: try both m_p as m_pvec and if none is available
return false
}

T* m_p;
std::vector<T*> m_pvec;
};

Sink<int> will compile, while Sink<double> not. Yes it can be rewritten with
an extra template argument, but that obfuscates code (no static_if in c++).
Anyway maybe we should drop Boost.Variant. It is also a compile time hog.