DM-9938: Make some afw types hashable #65
Conversation
| * particular, non-throwing) specialization of std::hash. | ||
| * | ||
| * @param seed An arbitrary starting value. | ||
| * @param value, rest The objects to hash. |
There was a problem hiding this comment.
I'm open to persuasion on this, but I'm not convinced it's valuable to document template parameters (via tparam) separately from regular arguments (via param) when the template parameters are expected to be completely inferred rather than explicitly provided. In those conditions, I feel like the actual template parameters are something of an implementation detail (albeit an unfortunately visible one, given the constraints of the language), and the documentation works better when the regular param docs just include information about permitted types.
There was a problem hiding this comment.
I don't see how template parameters are an implementation detail, given that templates are frequently used to represent flexible types rather than arbitrary ones. For example, often two parameters are required to be of the same type, or the output type and/or validity of future calls depends on the parameter. The programmer needs to be aware of such constraints.
Even in cases like this one where the parameters are almost unconstrained, it seems useful to separate type from value information. While you certainly could merge all the information from a type parameter description into the corresponding parameter documentation, that often leads to a single very long and hard to read paragraph. (And Doxygen doesn't let you have multiple paragraphs in a parameter description, part of the price you pay for being able to do things in whatever order you like.)
As for this being a C++ quirk, Java, which has a completely different implementation of generic programming, also documents type parameters.
There was a problem hiding this comment.
I'm by no means against documenting template parameters separately in general, and not very opposed to it at all in this case; I just think it's worth thinking about, especially in the case of variadics and overloads.
I've also been thinking about whether the case where template parameters must be explicitly provided (e.g. the first argument to std::make_shared) is sufficiently different from the case where they're inferred (all other make_shared arguments) that we should reserve @tparam to draw attention to the former case - but that's a general question about Doxygen usage, not a comment on this ticket, and I suppose it's something Doxygen would probably warn about anyway.
Anyhow, as an example of what I mean by "implementation detail", one could imagine implementing this API as:
template <typename ...Rest>
void hashCombine(int seed, Rest...rest);
...if, for some reason, we were forwarding to some other implementation that separated the first template argument from the rest of the parameter pack.
From the caller's perspective, that wouldn't shouldn't change the API, unless they were depending on the detailed signature in a way that I think we'd both agree is inadvisable.
Anyhow, I don't think it's a problem to document the function as it's written, especially because the signature you did use here does reflect what the function requires (and my counterfactual one does not!) and there are no non-template overloads that we'd like to maintain consistency with.
| template <typename T, typename... Rest> | ||
| std::size_t hashCombine(std::size_t seed, const T& value, Rest... rest) { | ||
| std::size_t hashCombine(std::size_t seed, const T& value, Rest... rest) noexcept { |
There was a problem hiding this comment.
Didn't spot this at first, but should this be Rest const &... rest (or Rest &&... rest with std::forward)?
There was a problem hiding this comment.
I don't know. I don't see why the former would hurt, given that the const T& and the recursive nature of the function impose that constraint on all arguments anyway (universal references, on the other hand, have too many gotchas for my liking). However, none of the cppreference examples use anything but a straight parameter name, so I think a case can be made that this is at least more idiomatic.
kfindeisen
force-pushed
the
tickets/DM-9938
branch
from
46d73b0
to
bae816e
Compare
The two overloads' comment blocks have been split up so that the requirements on the argument types can be documented. Missing indentation in the example has been added.
The template metaprogramming of HAS_STREAM_OUTPUT and printIfHashEqual is unfortunately necessary; by default, BOOST_TEST reports the unequal hashes but gives no indication of when the bug comes up.
The implementation is a generalization of the loop adopted for hashing geom::Point.
kfindeisen
force-pushed
the
tickets/DM-9938
branch
from
bae816e
to
86abfe6
Compare
This PR adds several extensions to the hash support added in #50.
I've added several functions for testing compliance with the
std::hashspec. I have not added a hash uniformity test. I found that GCC's implementation ofstd::hash<double>fails even a very naive bitwise test, and I don't want to require that implementers of classes whose state is a single primitive jump through hoops to meet a higher standard.