Use insert_same in insert_evaluation_cache
#73792
Conversation
|
r? @estebank (rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
crlf0710
added
T-compiler
|
☔ The latest upstream changes (presumably #75055) made this pull request unmergeable. Please resolve the merge conflicts. |
|
r? @eddyb |
GabrielMajeri
force-pushed
the
use-insert-same-in-insert-evaluation-cache
branch
3 times, most recently
from
8570de3 to
ce09d22
Compare
|
The code change looks correct, but deferring to @eddyb |
|
r? @nikomatsakis or @matthewjasper |
| // This should be changed to use HashMapExt::insert_same | ||
| // when that is fixed | ||
| self.tcx().evaluation_cache.insert(param_env.and(trait_ref), dep_node, result); | ||
| self.tcx().evaluation_cache.insert_same(param_env.and(trait_ref), dep_node, result); |
There was a problem hiding this comment.
So, #50507 is not yet closed -- but I think it is maybe specific to parallel execution? i.e., what prompted this PR?
There was a problem hiding this comment.
I was looking for issues to work on in the list at #48685, and found a reference to that issue. I began investigating the panic mentioned in the issue description, and was unable to reproduce it.
|
☔ The latest upstream changes (presumably #74862) made this pull request unmergeable. Please resolve the merge conflicts. |
GabrielMajeri
force-pushed
the
use-insert-same-in-insert-evaluation-cache
branch
from
ce09d22 to
53056bd
Compare
crlf0710
added
S-waiting-on-review
|
@nikomatsakis can we merge this or is this blocked on #50507 being merged first? |
|
Yeah, sorry, I got very distracted. I think I'd be inclined not to land this PR until #50507 is closed, just because it may cause unexpected problems if we were to enable the parallel mode. I guess the main thing would be to track down the problem -- though the bug report is pretty vague. In any case, it's borderline, but I'm inclined to just leave the code for now since it's just one line. @GabrielMajeri I do appreciate the PR though! Sorry for the confusion. :( |
Closes #50507.
I was trying to fix this issue, but it seems the reported crash doesn't happen anymore.