Uh oh!
There was an error while loading. Please reload this page.
Implement the Re-rebalance coherence RFC - #56145
Conversation
rust-highfive
commented
Nov 21, 2018
r? @varkor (rust_highfive has picked a reviewer for you, use r? to override) |
This comment has been minimized.
This comment has been minimized.
varkor
left a comment
There was a problem hiding this comment.
I haven't checked over the tests yet, but the code looks good. I'll try to look at the tests soon.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
That's directly copied from the RFC. I think it should be "if and only if" but's not written there. (cc @sgrif who has written that thing)
Uh oh!
There was an error while loading. Please reload this page.
weiznich
commented
Nov 27, 2018
@varkor I've addressed your review comments. |
This comment has been minimized.
This comment has been minimized.
varkor
commented
Dec 1, 2018
Sorry about the delay @weiznich. I'll get to this soon. |
6361b15 to
07328d5Compareweiznich
commented
Dec 3, 2018
@varkor I've rebased the PR. |
There was a problem hiding this comment.
You seem to have accidentally pulled in some extra changes with the rebase.
At the moment, the fact that all the coherence tests have been duplicated under the new feature flag seems a little excessive. I'm not quite sure what the correct approach here is though: we do want to make sure we don't break anything. I wonder if it would be appropriate to run the tests in two modes, a bit like NLL does at the moment. Alternatively, we could have a few tests that cover a representative sample of existing cases, under the assumption that the algorithm is mostly the same anyway and when stabilised, the existing tests are going to come into effect anyway. Could you point out which tests are new (and demonstrate the new behaviour), so I can check those in particular? I think @nikomatsakis will have better ideas about the right solution re. tests, so I'm going to reassign. The actual change looks good to me, though (once the rebase issues are fixed). |
weiznich
commented
Dec 6, 2018
This one and this one is new. All other tests are just a copied version of the old coherence tests with the
I'm not sure what would be a representative sample of the existing test cases here, nor I'm knowing what needs to be changed to run the existing tests using two modes. Would be great to get some more input here. |
06c2a37 to
8a97a74Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
8a97a74 to
852e95dCompareweiznich
commented
Dec 6, 2018
I'm not sure why those 2 tests fail. The corresponding |
This comment has been minimized.
This comment has been minimized.
weiznich
commented
Dec 11, 2018
@varkor@nikomatsakis Any news here? |
nikomatsakis
left a comment
There was a problem hiding this comment.
Seems great! I would like before r+'ing though to spend a bit more time checking over the tests.
There was a problem hiding this comment.
This comment needs to be //~^ -- no space
rust-highfive
commented
Dec 14, 2018
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
be925ae to
cf2f834Compareweiznich
commented
Dec 17, 2018
@nikomatsakis Tests are fixed. |
nikomatsakis
commented
Dec 19, 2018
I forgot about this question that @varkor raised:
One option here is to rewrite the duplicated tests using This will cause the same test to be built twice, once with You can then do things like: #![cfg_attr(new, feature(new_algorithm))]@weiznich -- do you want to take a shot at collapsing the tests this way? |
weiznich
commented
Dec 19, 2018
@nikomatsakis I will try that. What's the right strategy here? In that case there are already revisions. |
There was a problem hiding this comment.
Well, in this case you would have to add another set of three revisions, I suppose. e.g., a, re_a, b, re_b, c, re_c
bors
commented
Jan 3, 2019
☔ The latest upstream changes (presumably #55517) made this pull request unmergeable. Please resolve the merge conflicts. |
This copies and adjusts the existing coherence tests to ensure that they continue to work using the new implementation.
…ence.md Co-Authored-By: weiznich <Georg_semmler_05@web.de>
Implement compile tests as variants of existing tests
32118a9 to
d758e4dComparecramertj
commented
Jan 4, 2019
@bors r=nikomatsakis |
bors
commented
Jan 4, 2019
📌 Commit d758e4d has been approved by |
bors
commented
Jan 5, 2019
bors
commented
Jan 5, 2019
☀️ Test successful - status-appveyor, status-travis |
This is the first time I touch anything in the compiler so just tell me if I got something wrong.
Big thanks to @sgrif for the pointers where to look for those things.
cc #55437