[REG 2.084] hashOf will fail to compile for some structs/unions that recursively contain shared enums #3376
Conversation
n8sh
requested review from
andralex,
MartinNowak,
schveiguy and
wilzbach
as code owners
|
Thanks for your pull request, @n8sh! 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 + druntime#3376" |
test/hash/src/test_hash.d
Outdated
| // Also test the underlying reason the above was failing. | ||
| import core.internal.convert : toUbyte; | ||
| shared C c; | ||
| cast(void) toUbyte(c); |
There was a problem hiding this comment.
Instead of casting to void here and above, it's better to check if the return value is the expected one.
There was a problem hiding this comment.
Added a check here. Above there is no particular value the hash code is supposed to be.
There was a problem hiding this comment.
My motivation is the question: why would an optimizing compiler not remove a call to a pure function, if it's result is not used?
Technically such optimization happens after semantic analysis (where the bug manifests), in the backend, but in theory the frontend could also discard such function calls, so it's better to make the code more robust.
There was a problem hiding this comment.
Is there a change you would suggest that wouldn't make the test brittle?
There was a problem hiding this comment.
@PetarKirov Pure functions that may throw exceptions should not be optimized away. This pattern is prevalent in druntime: a function that does some checks and throws if the input is not the expected one.
|
@n8sh This seems to fail the tester pipeline. Any update on that? |
| @@ -769,7 +769,7 @@ const(ubyte)[] toUbyte(T)(const ref T val) if (is(T == enum)) | |||
| if (__ctfe) | |||
| { | |||
| static if (is(T V == enum)){} | |||
| return toUbyte(cast(const V) val); | |||
| return toUbyte(*cast(const V*) &val); | |||
There was a problem hiding this comment.
This change is reported to not be covered by any tests.
There was a problem hiding this comment.
The coverage report doesn't take into account the tests directory. It is covered by the new test added in this PR.
|
Accidentally rebased on master instead of stable. Fixing. |
n8sh
requested review from
DmitryOlshansky,
Geod24,
ibuclaw,
jacob-carlborg,
jmdavis,
TurkeyMan and
WalterBright
as code owners
…ructs/unions that recursively contain shared enums
No description provided.