-
Notifications
You must be signed in to change notification settings - Fork 12.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[DO NOT MERGE] Testing #70551 with de-abuse PRs #71939
Conversation
r? @davidtwco (rust_highfive has picked a reviewer for you, use r? to override) |
608df70
to
927c45d
Compare
Main reason is to simplify the code for what's to come, but also I don't know if that ever made a noticeable perf difference. The caching was introduced in 9c5e86d.
This is to clarify the API of VariantFields in preparation for filtering.
This was previously done by using tyerr as a fake type. That was hacky.
I had removed it a few commits ago to ease refactoring.
I introduced this mistake in 175976e and I can't quite remember what the reasoning was back then.
The idea is that `PatStack` is mostly used in the Matrix, and `Fields` is used whenever we're close to a relevant constructor. That way, we have a bunch of functions to go back and forth between a `Pat` and a `Constructor` along a `Fields`. That's a simpler mental model and better abstraction.
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 |
Try reverting "We don't use TyErr anymore", it should fix ci without changing anything else |
This reverts commit 9d9d3c9.
@Nadrieril Hmmm, that does seem to fix it. |
Yeah it does, but that's just a workaround. I could use that to get my own PR to build but the underlying bug might come back to bite us later |
Yes, and it also wouldn't resolve the abuse of tykind::err, which was the original point. At the same time, it would be nice to not have to fix this bug too before pushing in your changes... |
@Nadrieril I'm going to close this issue, but I can reopen and test other stuff if you want. |
It still solves the abuse of tyerr! The commit you reversed did something else that's not directly related to tyerr |
Oh, wow, I didn't even read the commit. Awesome! |
This is #70551 with #71930 and #71938 merged. I'm just using the CI to run tests because it's faster than my laptop...
Hopefully this confirms that those two PRs solve #70866...