Object.getHash() shouldn't be casting away const #3825
Conversation
|
Thanks for your pull request, @WalterBright! Bugzilla referencesYour PR doesn't reference any Bugzilla issue. If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog. 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 "master + druntime#3825" |
WalterBright
force-pushed
the
getHashConst
branch
from
2ec0f36 to
8fcebf0
Compare
WalterBright
force-pushed
the
getHashConst
branch
from
8fcebf0 to
acfb17e
Compare
src/core/thread/fiber.d
Outdated
| in | ||
| { | ||
| assert( dg ); | ||
| assert( cast(void delegate() const) dg ); |
There was a problem hiding this comment.
Why is this cast needed? It looks like a simple null check.
There was a problem hiding this comment.
It isn't a simple null check. It winds up calling dassert() which has overloads for delegates.
There was a problem hiding this comment.
That looks like an issue on its own, assert(dg) should just compile. Is there a bugzilla issue for that or am I missing something?
There was a problem hiding this comment.
assert's inside contracts behave differently.
There was a problem hiding this comment.
assert's inside contracts behave differently.
Can you elaborate?
There was a problem hiding this comment.
Here's the error if I leave it unchanged:
src/core/thread/fiber.d(609): Error: none of the overloads of template `core.internal.dassert._d_assert_fail` are callable using argument types `!(void delegate())(string, void delegate())`
src/core/internal/dassert.d(38): Candidates are: `_d_assert_fail(A)(scope const string op, auto ref scope const A a)`
src/core/internal/dassert.d(63): `_d_assert_fail(A...)`
There was a problem hiding this comment.
It looks like we should add that overload on druntime. Does it make sense to the user to cast it exolicitly to const?
|
The buildkite errors don't seem to be related to this PR. |
|
It's hard to see how the buildkite errors have anything to do with this PR. |
WalterBright
force-pushed
the
getHashConst
branch
4 times, most recently
from
51e3eb0 to
1806121
Compare
Yeah, it's not. See vibe-d/vibe.d#2661 for context. |
src/core/thread/fiber.d
Outdated
| { | ||
| assert( dg ); | ||
| // assert( cast(void delegate() const) dg ); | ||
| // assert( dg ); |
There was a problem hiding this comment.
Why not just delete this code instead of leaving it commented?
There was a problem hiding this comment.
Because I don't know what is triggering the buildkite failures, or if those failures are unrelated to this PR.
There was a problem hiding this comment.
The vibe-d and mir-algorithm failures are unrelated and being worked on
WalterBright
force-pushed
the
getHashConst
branch
from
1806121 to
ca04bd5
Compare
|
buildkike/vibe is still failing, although the libmir problem has disappeared. |
|
Vibe.d has been fixed. Please remove the code instead of commenting it tho. |
WalterBright
force-pushed
the
getHashConst
branch
from
ca04bd5 to
9f85542
Compare
|
It's removed now. |
6 participants
blocking dlang/dmd#14164