fix Issue 16311 - toHash for Slice is not defined #4639
Conversation
std/experimental/ndslice/slice.d
Outdated
| See_also: $(ZREF2 Slice.toMurmurHash3, toMurmurHash3), $(MREF std, _digest, murmurhash) | ||
| +/ | ||
| pure nothrow @nogc | ||
| size_t toHash()() const @safe |
There was a problem hiding this comment.
Why is it a template ?
And if it is, it shouldn't have explicit attribute but const.
|
LGTM |
std/experimental/ndslice/slice.d
Outdated
| Hash map implementations should care about finalization step. | ||
| Built-in associative arrays has finalization step. | ||
|
|
||
| See_also: $(ZREF2 Slice.toMurmurHash3, toMurmurHash3), $(MREF std, _digest, murmurhash) |
There was a problem hiding this comment.
Do you know the REF_ALTTEXT macro? The reference here won't work with ddox
|
Just a quick glance, but many functions have parameters with no |
Added |
|
|
||
| /++ | ||
| Computes hash value using MurmurHash3 algorithms without the finalization step. | ||
|
|
There was a problem hiding this comment.
Missing Params section. Document things like "32 or 128" here.
There was a problem hiding this comment.
The proper description of size and opt is large enough. I added reference to MMH3 in the See_also section Instead of Params.
|
LGTM |
| { | ||
| import std.experimental.ndslice.selection : iotaSlice; | ||
| const sl = iotaSlice(3, 7); | ||
| size_t hash = sl.toHash; |
There was a problem hiding this comment.
small FYI - using assert on ddoced tests isn't bad - in fact a soon-to-come update might rewrite the asserts to writeln in interactive mode (see dlang/dlang.org#1297)
|
wow this is some kickass work 🎉 |
|
@9il would you be so kind to rebase this, so that we can see the coverage report. |
Current coverage is 88.70% (diff: 92.59%)@@ master #4639 diff @@
==========================================
Files 121 121
Lines 73916 73968 +52
Methods 0 0
Messages 0 0
Branches 0 0
==========================================
+ Hits 65566 65612 +46
- Misses 8350 8356 +6
Partials 0 0
|
std/digest/murmurhash.d
Outdated
| h3 = seed[3 - 1]; | ||
| h2 = seed[2 - 1]; | ||
| h1 = seed[1 - 1]; | ||
| } |
There was a problem hiding this comment.
according to coverage, this is never tested.
There was a problem hiding this comment.
ok, i will just remove them. If some one wants them, then he can open PR and write unittests :-)
|
Why is the default |
Default |
|
Coverage equals 100%. Codecove shows attribute propagation in |
|
rabased |
Yep (one step at a time) -> #4699 |
|
@DmitryOlshansky Can we move forward with this PR? Martin will merge to the stable branch soon, thought. |
|
Auto-merge toggled on |
|
Thanks! |
8 participants
The code has grouping of elements. And optimisation in case of
stride!(N-1) == 1.Coverage equals 100%. Codecove shows attribute propagation in
murmurhash.das not covered for no reason (probably because Travis is always 64-bit for Phobos).