Hash: Refresh for external use. #4481
Conversation
| Caller is responsible for freeing ring node memory. | ||
| */ | ||
|
|
||
| struct ATSConsistentHash { |
There was a problem hiding this comment.
As long as you're in here, why is this a struct not a class?
There was a problem hiding this comment.
You'd have to ask @jrushford - I wanted to minimize the changes in this area, it's really outside the scope of the PR.
| virtual self_type &update(std::string_view const &data) = 0; | ||
|
|
||
| /// Finalize the hash function output. | ||
| virtual self_type & final() = 0; |
There was a problem hiding this comment.
Interesting. Although final is a keyword, it's not a reserve word. Consistency is the hobgoblin of little programming languages.
There was a problem hiding this comment.
We could change that, it's a legacy from pre-eleventy days.
| }; | ||
|
|
||
| /// A hash function that returns a 32 bit result. | ||
| struct Hash32Functor : HashFunctor { |
There was a problem hiding this comment.
OK I looked it up structures, unlike classes, have public inheritance by default. Or, you could say public and people would not have to look it up.
There was a problem hiding this comment.
That's an interview question - you shouldn't have to look it up. It's less obscure than "final" being a keyword.
There was a problem hiding this comment.
Haha too late already hired me.
There was a problem hiding this comment.
I like struct for that exact reason, it instantly tells me that it's all public, and I don't think we should discourage the use of struct over class.
There was a problem hiding this comment.
@zwoop I was referring to using default access control for inheritance. I prefer it explicit, unlike SolidWallOfObscurity.
There was a problem hiding this comment.
I made another comment above, questioning the use of struct instead of class, when the class has some members with private or protected access control.
There was a problem hiding this comment.
In this particular case, it's inertia. When I write new code, I tend to follow Leif's approach and only use struct for POD things.
| * The main purpose of this is to allow run time changes in hashing, which is required in various | ||
| * circumstances. | ||
| */ | ||
| struct HashFunctor { |
There was a problem hiding this comment.
Aren't functors supposed to have a function call operator?
There was a problem hiding this comment.
I'm open to suggestions for a better name. I thought "Hash" was too generic. OTOH the point of this is to embed a function, which makes it a functor.
There was a problem hiding this comment.
HashBase? HashSuper? HashSmoker?
There was a problem hiding this comment.
"HashFunction", except you'd complain it wasn't a function. "HashFunctionContainer" :-). "HashWrap".
There was a problem hiding this comment.
HashFunctorish
HashFunctoresque
HashFunkyFunctor
HashFlunktor
| virtual size_t size() const = 0; | ||
|
|
||
| /// Copy the result to @a dst. | ||
| /// @a dst must be at least @c result_size bytes long. |
There was a problem hiding this comment.
There was a problem hiding this comment.
Artifact from some API reshuffling.
|
|
||
| /// Copy the result to @a dst. | ||
| /// @a dst must be at least @c result_size bytes long. | ||
| /// @return @c true if the result was copied to @a data, @c false otherwise. |
|
I shuffled the API around quite a bit working on this, I need to review the comments and make sure they're consistent with the final API. |
|
|
||
| template <typename X, typename V> self_type &update(TransformView<X, V> view); | ||
|
|
||
| template <typename X, typename V> value_type hash_immediate(TransformView<X, V> const &view); |
There was a problem hiding this comment.
Why does this take a reference rather than a value param like update()?
There was a problem hiding this comment.
Because update uses the parameter as local variable and hash_immediate doesn't. It shouldn't matter from a caller's point of view.
|
|
||
| /// Check if view is empty. | ||
| bool empty() const; | ||
| /// Check if bool is not empty. |
SolidWallOfCode
force-pushed
the
hash-function-refresh
branch
2 times, most recently
from
98bd959 to
b318330
Compare
This updates the API for hash functions in ATS to make it suitable for external (plugin) use. The basic hashes are moved to the extern C++ area.
SolidWallOfCode
force-pushed
the
hash-function-refresh
branch
from
b318330 to
b70c4d7
Compare
4 participants
This updates the API for hash functions in ATS to make it suitable for external (plugin) use.
The basic hashes are moved to the extern C++ area. The more obscure hashes are kept in core, and MD5 isn't exported because that would create a dependency on the openSSL library.
Also, I need this for the YAML work.
Depends on #4480.