[hash] Turn on BOOST_HASH_NO_IMPLICIT_CASTS by default
Hello everyone, 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. 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. 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. 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. Of course, there will be a macro to keep the old behaviour. Any objections to this change?
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@sig.com 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.
Stewart, Robert wrote:
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.
Some stdlib implementers have chosen to provide a primary std::hash template that is SFINAEd on is_integral || is_enum. We could do the same for hash_value.
Thanks for replying. A lot of food for thought. On 17 April 2012 15:37, Stewart, Robert <Robert.Stewart@sig.com> wrote:
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.
There's actually no ODR risk. The only difference is whether the function to prevent implicit conversions is present or not. If it isn't instantiated then the two versions of the header are effectively identical. If it is instantiated the result is a compile error.
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.
I'm not very keen on changing things in the public interface. Also, I'm still supporting some of the less capable compilers which can be a bit odd about SFINAE. Although I'm now using SFINAE in the unordered containers and with a bit effort it's working okay in all the test compilers. I suppose the big question is, which implicit casts should considered suitable. IIUC your solution has the advantage of only banning certain implicit casts. Even if I wanted to avoid SFINAE, I could probably write a type trait to detect them that doesn't rely on it. I'm not sure if an implicit cast to a class' hash_function would always be considered safe. For example, someone might supply a lossy implicit conversion to std::string.
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.
That's an interesting idea regardless of this issue. But I'm not sure how much you can infer from a numeric_limits specialisation. My worry is that someone might specialise it for a class that gives the hash function problems, something like a big int perhaps.
Daniel James wrote:
On 17 April 2012 15:37, Stewart, Robert <Robert.Stewart@sig.com> wrote:
I suppose the big question is, which implicit casts should considered suitable. IIUC your solution has the advantage of only banning certain implicit casts.
Right. By restricting the set of types that are supported to the exact types mentioned, there can be no implicit conversions to those types. Nevertheless, the primary specialization can permit implicit conversions for UDTs.
Even if I wanted to avoid SFINAE, I could probably write a type trait to detect them that doesn't rely on it.
By that I presume you mean that you would use a trait to dispatch to other functions/function templates that avoid the bool conversion problem, right?
I'm not sure if an implicit cast to a class' hash_function would always be considered safe. For example, someone might supply a lossy implicit conversion to std::string.
Implicit conversions cause all manner of problems. They are rarely a good idea, so I doubt you should worry about that. Besides, the function template can be specialized for such a class when the implicit conversion causes undesirable hashing.
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.
That's an interesting idea regardless of this issue. But I'm not sure how much you can infer from a numeric_limits specialisation. My worry is that someone might specialise it for a class that gives the hash function problems, something like a big int perhaps.
The function template can be, and very likely should be, specialized for the big int type, so any issues can be rectified, though not prevented with that approach. _____ Rob Stewart robert.stewart@sig.com 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.
On 18 April 2012 12:44, Stewart, Robert <Robert.Stewart@sig.com> wrote:
Daniel James wrote:
Even if I wanted to avoid SFINAE, I could probably write a type trait to detect them that doesn't rely on it.
By that I presume you mean that you would use a trait to dispatch to other functions/function templates that avoid the bool conversion problem, right?
Yes. I tried implementing both, and the SFINAE version was simpler than the traits version, so I've checked that in, and it seems to have worked without any issues on all of our test compilers. So I think I'll go with that. I haven't finished it yet, there are still some other built in types to deal with. I'm a little worried about changing the actual signature of the functions, the nice thing about a traits based version is that it would leave the API alone. Documenting this cleanly will also be an issue. It also clashes with the BOOST_HASH_NO_IMPLICIT mechanism because it makes the calls to template functions ambiguous. But if this works then it's redundant and fine to remove it.
I'm not sure if an implicit cast to a class' hash_function would always be considered safe. For example, someone might supply a lossy implicit conversion to std::string.
Implicit conversions cause all manner of problems. They are rarely a good idea, so I doubt you should worry about that. Besides, the function template can be specialized for such a class when the implicit conversion causes undesirable hashing.
I reread the original design paper and it deliberately supports implicit conversions to base, so maybe that should stay.
participants (3)
-
Daniel James -
Peter Dimov -
Stewart, Robert