Add tests and docs for #[derive(GenericTypeVisitable)] - #161806
Conversation
|
|
rustbot
added
A-rustc-dev-guide
|
r? @mejrs rustbot has assigned @mejrs. Use Why was this reviewer chosen?The reviewer was selected based on:
|
|
r? me |
|
|
||
| #[derive(GenericTypeVisitable)] | ||
| struct MissingBound<T> { | ||
| // This should fail, as `T: GenericTypeVisitable<__V>` wasn't specified |
There was a problem hiding this comment.
I guess you need to split this into it's own file then
rustbot
added
S-waiting-on-author
|
Reminder, once the PR becomes ready for a review, use |
|
Other than that this looks good to me, thanks! |
This comment has been minimized.
This comment has been minimized.
dbcc3e0 to
a570f6b
Compare
|
|
||
| unsafe impl<V> GenericTypeVisitable<V> for Incrementer { | ||
| fn generic_visit_with(&self, _visitor: &mut V) { | ||
| COUNT.fetch_add(1, Ordering::AcqRel); |
There was a problem hiding this comment.
nit: Ordering::Relaxed should be fine here and below.
This ofc doesn't matter for performance because this is merely a test, but imo using anything other than Relaxed should only be done if there is a reason for it, as otherwise it might be unnecessarily confusing to future readers
There was a problem hiding this comment.
I was just being extra cautious.. atomics scary. But given that everything here is happening in one thread, Relaxed should indeed be fine.
There was a problem hiding this comment.
Even if there were multiple threads, we don't rely on the ordering of reads/writes of COUNT relative to other atomic operations, so Relaxed should be fine
There was a problem hiding this comment.
Two threads fetch_add-ing the value of COUNT at the same time with Ordering::Relaxed would race though, no?
|
(also please rebase) |
I ended up deciding not to add docs about `bounds` as it seems like a relatively minor feature of the derive, and there are docs at [1]. [1]: https://github.com/rust-lang/rust/blob/3ffb26fbf5bf232cf59e314e75ea325973f4f583/compiler/rustc_type_ir_macros/src/lib.rs#L21-L55
Just specifying `T: GenericTypeVisitable` doesn't work, as the trait has a generic: `V`, the visitor. `T: GenericTypeVisitable<__V>` is what actually works, as `__V` is the generic added to the impl generated by the derive macro. We discussed[1] different ways of making this nicer, but settled on not doing anything, as we don't expect people to need to specify any actual bounds. [1]: https://rust-lang.zulipchat.com/#narrow/channel/185405-t-compiler.2Frust-analyzer/topic/Updating.20next-solver/near/618331780 and below
a570f6b to
12dd807
Compare
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
12dd807 to
ada4f67
Compare
|
(forgot to actually |
|
@rustbot ready (it never feels right to post this before the CI is green, but hey, it passes on my machine so it should be fine right :p ) |
rustbot
added
S-waiting-on-review
4 participants
..given the added complexity from the newly-added
boundsattributeFollow-up to #160914
More details in individual commits.
cc @JonathanBrouwer (you might want to take over the review of this since you have some context already.. but as you wish)
cc @ChayimFriedman2