make it so grapheme can be used as an aa key #8855
Conversation
|
Thanks for your pull request, @schveiguy! Bugzilla references
Testing this PR locallyIf you don't have a local development environment setup, you can use Digger to test this PR: dub run digger -- build "stable + phobos#8855" |
schveiguy
force-pushed
the
graphemehash
branch
from
6caca21
to
5331a2c
Compare
|
@schveiguy the CI is green! |
|
Yeah, I need an issue report. @kinke had a use case (see #8787 (review)), I'll see if I can test this and make a report tomorrow. |
|
[The use case is https://github.com/adamdruppe/arsd/blob/410216f649f192f7bedda140f121deb00fd7fbc8/terminal.d#L2276] I haven't filed an issue, since the regression was introduced for beta/RC releases only so far. |
| // Define a default toHash to allow AA usage | ||
| size_t toHash() const @trusted | ||
| { | ||
| return hashOf(slen_, hashOf(small_)); |
There was a problem hiding this comment.
AFAICT, this only works for the small-case, not for the heap-buffer case.
There was a problem hiding this comment.
The heap buffer data doesn't matter. The opEquals is just comparing the pointers.
Not saying this is what we want it to be, but that is what it is now.
There was a problem hiding this comment.
Yeah I know that the opEquals doesn't work in that case either, and that both implicit opEquals and toHash before v2.106 were most likely working in that case only too (IIRC, unions are by-default compared and hashed as a binary blob of the union's size).
Ideally, we'd be comparing/hashing the underlying bytes slice. But as the [s]len_ fields seem to be the number of dchars, a proper comparison/hash might have to rely on the dchar range returned by opSlice().
There was a problem hiding this comment.
I don't actually know what the tupleof comparison does for the union...
There was a problem hiding this comment.
FWIW, I think the proper opEquals and toHash should be based on the opSlice.
There was a problem hiding this comment.
After a quick check, it checks all members of the union, i.e., the same payload multiple times. For the pointers and integers here, it amounts to something like a double-memcmp, instead of a single memcmp for the implicit union comparison.
There was a problem hiding this comment.
"it" being the default opEquals or being tupleof == tupleof?
There was a problem hiding this comment.
The new tupleof comparison. :)
There was a problem hiding this comment.
wow. I just tested it out. And this is pretty dangerous. For example, if one of the union members (that isn't valid) has an opEquals, then using the tupleof calls this, the default does a memcmp.
In this case of course, it's not a problem. But for sure this is not a good implementation.
There was a problem hiding this comment.
Oh, that reminds me of https://issues.dlang.org/show_bug.cgi?id=22171 - the same problem for autogenerated __xtoHash, which uses all union members.
schveiguy
force-pushed
the
graphemehash
branch
from
5331a2c
to
0fdbc2c
Compare
|
OK, bug report added and ready to close. |
Need an issue report. Just want to make sure it passes CI