Re: [boost] [hash] Turn on BOOST_HASH_NO_IMPLICIT_CASTS by default

Subject: Re: [boost] [hash] Turn on BOOST_HASH_NO_IMPLICIT_CASTS by default
From: Stewart, Robert (Robert.Stewart_at_[hidden])
Date: 2012-04-17 10:37:31

Daniel James wrote:
>
> There have been occasional issues where Boost.Hash will
> incorrectly hash an object that doesn't have a hash function
> but is convertible to bool.

That's bad and should certainly be fixed.

> I added an extra templated overload of hash_value which
> contains a static assert. The idea is that it's a better match
> than the implicit cast, so it will cause a compile error in
> this case. This is currently only activated when a macro is
> defined, as it could break otherwise valid code. I was a bit
> worried about making any interface changes because at this
> stage, Boost.Hash should be very stable from release to
> release. The problem turned up again yesterday, so I'm starting
> to think it's enough of a problem that it's worth activating
> this be default.

I applaud your desire to avoid breaking code, but there may be a
problem.  If the header is included in different places with
different definitions for the macro, won't that violate the ODR?
All code linked into an application, whether statically or
dynamically, must use the same definition for the macro.  I can
imagine many scenarios which would violate that precondition
without warning.

> A different solution might be to change the implement of
> Boost.Hash for bools, so that the bool overload isn't
> required. But that would be treating the sympton, as other
> implicit casts could cause problems.

You can make all of the existing overloads into function
templates and use SFINAE to select the specific type in each
case.  That is, whereas there is now a bool overload, make it a
function template that uses SFINAE to enable it only when the
template parameter is bool.  That approach would disallow any
implicit conversions to the built-in types for which there are
now overloads.  That is a breaking change, of course.

You could also use std::numeric_limits<T>::is_specialized to
detect a possibly larger set of types to which overloads in the
details namespace could be applied.

> An example of valid code that will now break is something
> like:
>
>     struct base;
>     std::size_t hash_value(base const&);
>
>     struct derived : base
>     {
>         void some_method_but_no_extra_state();
>     };
>
>     boost::hash<derived> derived_hash;
>
> Currently that will automatically use base's hash_value, with
> this change it will be a compile error. Although that might be
> a good thing, as sometime it is an error to use the base's hash
> function.

Forcing a conscious choice is a good thing and it applies to my
function template + SFINAE idea, too.  Unfortunately, this does
mean many currently valid uses will no longer compile.

_____
Rob Stewart                           robert.stewart_at_[hidden]
Software Engineer                     using std::disclaimer;
Dev Tools & Components
Susquehanna International Group, LLP  http://www.sig.com

________________________________

IMPORTANT: The information contained in this email and/or its attachments is confidential. If you are not the intended recipient, please notify the sender immediately by reply and immediately delete this message and all its attachments. Any review, use, reproduction, disclosure or dissemination of this message or any attachment by an unintended recipient is strictly prohibited. Neither this message nor any attachment is intended as or should be construed as an offer, solicitation or recommendation to buy or sell any security or other financial instrument. Neither the sender, his or her employer nor any of their respective affiliates makes any warranties as to the completeness or accuracy of any of the information contained herein or that this message or any of its attachments is free of viruses.