Re: [boost] [hash] documentation issues (was [TR1 Review] TR1 Formal Review Beginning)
Daniel James wrote:
Alexander Nasonov wrote:
2. Adding new has_value may have side effects, for example, (a) hash<T> may compile for not customized type T although T doesn't necesarily have properly defined equality function; or (b) it may introduce overload resolution ambiguities, in worst case, hash<type_from_6_3_3_1> may stop working.
I don't think this is nearly as bad as you think. Any type from TR1 will only stop working if you declare hash_value in the boost or std namespace - which you really shouldn't do.
You are right. I was a way too pessimistic.
If a type has a hash_value function but no equality then boost::hash will compile for it - but a hashed container won't, because it will require the equality function. So the only time will boost::hash will work is if it's used in another context where equality isn't required.
That's not quite right. For example #include <iostream> #include <boost/functional/hash.hpp> struct Base { bool operator==(Base) const { return true; } }; std::size_t hash_value(Base) { return 0; // Base has no state } struct Derived : Base { int dummy; bool operator==(Derived other) const { return dummy == other.dummy; } }; int main() { Derived x; std::cout << boost::hash<Derived>()(x) << '\n'; } -- Alexander Nasonov
Alexander Nasonov wrote:
That's not quite right. For example
#include <iostream> #include <boost/functional/hash.hpp>
struct Base { bool operator==(Base) const { return true; } };
std::size_t hash_value(Base) { return 0; // Base has no state }
struct Derived : Base { int dummy; bool operator==(Derived other) const { return dummy == other.dummy; } };
OK, I thought you meant something else. It seems to me that in this case, hash_value(Base) is a part of Base's interface - so any class that derives from it should take it into account. It also appears that Base wasn't really designed for derivation (despite its name ;), and it's generally a bad idea to derive from such classes. I guess that's a bit of a cop out answer, but this is more of a problem with the way the derivation is done than Boost.Hash itself. What I should do, is add some documentation about using Boost.Hash with class inheritance. I'll try to do that soon-ish. What do you think about having a macro to disable the extensions? Am I right in thinking that your main concern is that the extensions are in Boost.TR1? Sorry for the late reply, Daniel
Daniel James wrote:
What I should do, is add some documentation about using Boost.Hash with class inheritance. I'll try to do that soon-ish. Great!
What do you think about having a macro to disable the extensions? It would be nice addition to Boost.Hash.
Am I right in thinking that your main concern is that the extensions are in Boost.TR1? Yes
-- Alexander Nasonov
Alexander Nasonov wrote:
Daniel James wrote:
What I should do, is add some documentation about using Boost.Hash with class inheritance. I'll try to do that soon-ish. Great!
What do you think about having a macro to disable the extensions? It would be nice addition to Boost.Hash.
Am I right in thinking that your main concern is that the extensions are in Boost.TR1? Yes
I'd like to add that having the extensions in the Boost implementation of TR1 is a good way to uncover potential problems with them, in preparation for their official submission for TR1.5 or TR2. My preference would be to leave them in.
What do you think about having a macro to disable the extensions? Am I right in thinking that your main concern is that the extensions are in Boost.TR1?
I'm not sure that would work unfortunately: if I have one library that uses the TR1 <functional> and another that uses <boost/functional/hash.hpp> and then include both libraries in my code, will they both work? Which "version" of the hash lib do I get? John.
John Maddock wrote:
What do you think about having a macro to disable the extensions? Am I right in thinking that your main concern is that the extensions are in Boost.TR1?
I'm not sure that would work unfortunately: if I have one library that uses the TR1 <functional> and another that uses <boost/functional/hash.hpp> and then include both libraries in my code, will they both work? Which "version" of the hash lib do I get?
The extensions would only be disabled if the user defines the macro, I don't think they should be disabled by default. If you're worried about ODR conflicts they can easily be avoided. The only cost is that the class is now specialized for the types mentioned in TR1, instead of just having a single definition. Daniel
Daniel James wrote:
John Maddock wrote:
I'm not sure that would work unfortunately: if I have one library that uses the TR1 <functional> and another that uses <boost/functional/hash.hpp> and then include both libraries in my code, will they both work? Which "version" of the hash lib do I get?
The extensions would only be disabled if the user defines the macro, I don't think they should be disabled by default. If you're worried about ODR conflicts they can easily be avoided. The only cost is that the class is now specialized for the types mentioned in TR1, instead of just having a single definition.
I guess there is a problem with something like: a.hpp: #define BOOST_HASH_NO_EXTENSIONS #include <functional> // .... b.hpp: #include <functional> // .... a.cpp: #include "a.hpp" #include "b.hpp" But this would really be against the intent of the macro - which is to check that your code is not inadvertantly making use of the extensions. I will suggest in the documentation that the macro is only used as a compiler switch, and never in headers. Or, I could just have two classes boost::hash & boost::tr1::hash (or whatever). All of the implementation will still be in hash_value etc. so there wouldn't be any code bloat. The macro would only affect boost::tr1::hash, which would still include the extensions by default. But that seems overly complicated. Just so you understand what I'm getting at, here's some (untested and probably incorrect) code. namespace boost { template <class T> struct hash; template <> struct hash<int> : public std::unary_function<int, std::size_t> { std::size operator()(int v) const { return boost::hash_value(v); } }; // etc. #if !defined(BOOST_HASH_NO_EXTENSIONS) template <class T> struct hash<T> : public std::unary_function<T, std::size_t> { std::size operator()(T v) const { return hash_value(v); } }; #endif } Can you see any other problems with it? Daniel
Daniel James wrote:
I guess there is a problem with something like:
a.hpp:
#define BOOST_HASH_NO_EXTENSIONS #include <functional> // ....
b.hpp:
#include <functional> // ....
a.cpp:
#include "a.hpp" #include "b.hpp"
But this would really be against the intent of the macro - which is to check that your code is not inadvertantly making use of the extensions. I will suggest in the documentation that the macro is only used as a compiler switch, and never in headers.
What if some other Boost lib makes use of the extensions internally, so that the user shouldn't have to care? Should Boost libs avoid the extension so that they still work if the user defines the macro?
Or, I could just have two classes boost::hash & boost::tr1::hash (or whatever). All of the implementation will still be in hash_value etc. so there wouldn't be any code bloat. The macro would only affect boost::tr1::hash, which would still include the extensions by default. But that seems overly complicated.
Boost libs could use the extensions under this scheme. jon
Jonathan Wakely wrote:
What if some other Boost lib makes use of the extensions internally, so that the user shouldn't have to care? Should Boost libs avoid the extension so that they still work if the user defines the macro?
Or, I could just have two classes boost::hash & boost::tr1::hash (or whatever). All of the implementation will still be in hash_value etc. so there wouldn't be any code bloat. The macro would only affect boost::tr1::hash, which would still include the extensions by default. But that seems overly complicated.
Boost libs could use the extensions under this scheme.
All true. But there will still be the same problem if anyone uses the extensions with Boost.TR1. This would inevitably lead to the recommendation (but not from me) that if you want to use the extensions you should use boost::hash, not std::tr1::hash. But I was convinced by Peter and John's arguments (which I linked to before) that the extensions should be in Boost.TR1. Personally, I would see the macro as being similar to 'strict', 'ansi' or 'no-extensions' compiler switches which I do find very useful, mainly for unit tests where you can generally avoid these kinds of conflicts. So the macro might have some use for when you want to guarantee that your code will work on other implementations of TR1. If you find that some other library needs the extensions - you can just not define the macro (anything which works without the extensions will also work with them). I believe that the only time this would really be a problem is if the macro is defined in a header the you can't change. Which is why I would suggest that it is never used in that way. Daniel
All true. But there will still be the same problem if anyone uses the extensions with Boost.TR1. This would inevitably lead to the recommendation (but not from me) that if you want to use the extensions you should use boost::hash, not std::tr1::hash. But I was convinced by Peter and John's arguments (which I linked to before) that the extensions should be in Boost.TR1.
Personally, I would see the macro as being similar to 'strict', 'ansi' or 'no-extensions' compiler switches which I do find very useful, mainly for unit tests where you can generally avoid these kinds of conflicts. So the macro might have some use for when you want to guarantee that your code will work on other implementations of TR1.
I could go with that. John.
participants (5)
-
Alexander Nasonov -
Daniel James -
John Maddock -
Jonathan Wakely -
Peter Dimov