Use query for Variant InhabitedPredicate#159541
Conversation
|
Some changes occurred in exhaustiveness checking cc @Nadrieril Some changes occurred in match checking cc @Nadrieril |
|
r? @folkertdev rustbot has assigned @folkertdev. Use Why was this reviewer chosen?The reviewer was selected based on:
|
This comment has been minimized.
This comment has been minimized.
|
@rustbot author |
|
Reminder, once the PR becomes ready for a review, use |
| let adt = tcx.adt_def(def_id); | ||
| variant_inhabited_predicate(tcx, adt.non_enum_variant()) | ||
| } | ||
| DefKind::Variant => { |
There was a problem hiding this comment.
Feels unexpected to me that a Variant would be an admissible input for a function called something_adt; this deserves an extra comment imo
There was a problem hiding this comment.
I think I'd prefer to rename the query to inhabited_predicate_for_def and have it return an EarlyBinder<InhabitedPredicate>
There was a problem hiding this comment.
I did the rename but still looking into EarlyBinder. Maybe that can be a separate PR?
There was a problem hiding this comment.
It's just a wrapper that says "btw, the value you get from this query needs to be passed generic arguments", it's what we use for the output of most queries that get type-related information about a DefId, just as a reminder
There was a problem hiding this comment.
Also happy to leave that, it's not crucial
There was a problem hiding this comment.
Since both are potentially performance-affecting, let's move to TypeFolder later?
There was a problem hiding this comment.
Okay how bout we merge this as is and I'll add EarlyBinder with or without TypeFolder separately.
There was a problem hiding this comment.
Oh it seems I can't just use EarlyBinder by itself because EarlyBinder::bind requires TypeFoldable.
Also I just found out that TypeFoldable requires TypeVisitable, so I don't think that's practical.
There was a problem hiding this comment.
oh! well, let's just leave things like this then
|
r? me I left a few comments as I was passing by but the most important is the perf run ofc |
2c18d60 to
553f6c8
Compare
|
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Use query for Variant InhabitedPredicate
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (2421524): comparison URL. Overall result: ✅ improvements - no action neededBenchmarking means the PR may be perf-sensitive. It's automatically marked not fit for rolling up. Overriding is possible but disadvised: it risks changing compiler perf. @bors rollup=never rustc-perf Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (primary 0.9%, secondary -0.1%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary 2.1%, secondary -2.5%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeResults (secondary -0.1%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Bootstrap: 488.235s -> 485.708s (-0.52%) |
|
Small but with a green tendency, I say why not |
|
@bors r+ |
View all comments
It looks like there may be some benefit from caching.