Add default to TypeFoldable::fold_with - #159584
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
This comment has been minimized.
This comment has been minimized.
23af8de to
a915f8c
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| /// infallible folders. Do not override this method, to ensure coherence | ||
| /// with `try_super_fold_with`. | ||
| fn super_fold_with<F: TypeFolder<I>>(self, folder: &mut F) -> Self; | ||
| fn super_fold_with<F: TypeFolder<I>>(self, folder: &mut F) -> Self { |
There was a problem hiding this comment.
Did you test #[inline] on it locally?
There was a problem hiding this comment.
I didn't test locally. Added #[inline] here and also to the new struct methods. Wanna try another perf run?
| /// the behavior in sync across functions. | ||
| fn fold_with<F: TypeFolder<I>>(self, folder: &mut F) -> Self; | ||
| #[inline] | ||
| fn fold_with<F: TypeFolder<I>>(self, folder: &mut F) -> Self { |
There was a problem hiding this comment.
(Ideally we would mark this final but unfortunately this crate needs to be able to compile on stable, too. We could make it conditional on feature="nightly" using an identity macro but at this point we should just give up I guess.)
There was a problem hiding this comment.
I went ahead and did this the naive way. Doesn't seem like too much copy-paste?
| /// infallible folders. Do not override this method, to ensure coherence | ||
| /// with `try_super_fold_with`. | ||
| fn super_fold_with<F: TypeFolder<I>>(self, folder: &mut F) -> Self; | ||
| fn super_fold_with<F: TypeFolder<I>>(self, folder: &mut F) -> Self { |
a915f8c to
12bd193
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. |
|
@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.
Add default to `TypeFoldable::fold_with`
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (72fafa6): comparison URL. Overall result: ❌✅ regressions and improvements - please read:Benchmarking 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. Next, please: If you can, justify the regressions found in this try perf run in writing along with @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)This perf run didn't have relevant results for this metric. CyclesResults (primary 7.2%, secondary 8.1%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeThis perf run didn't have relevant results for this metric. Bootstrap: 469.882s -> 474.787s (1.04%) |
12bd193 to
80eb019
Compare
80eb019 to
14eda03
Compare
| @@ -94,7 +94,19 @@ pub trait TypeFoldable<I: Interner>: TypeVisitable<I> + Clone { | |||
| /// | |||
| /// Same as [`TypeFoldable::try_fold_with`], but not fallible. Make sure to keep | |||
| /// the behavior in sync across functions. | |||
There was a problem hiding this comment.
I should probably delete this sentence.
| /// the behavior in sync across functions. | ||
| fn fold_with<F: TypeFolder<I>>(self, folder: &mut F) -> Self; | ||
| #[inline] | ||
| #[cfg(feature = "nightly")] // for `final` |
There was a problem hiding this comment.
That's not sufficient unfortunately. final like all recent unstable syntax is gated pre-expansion, meaning this will still lead to a feature gate error if feature = "nightly" is not set.
Since we're now building+testing (a subset of?) rust-analyzer in CI (which uses the stable version of this crate), this should also manifest itself as a CI failure soon IINM.
In https://github.com/rust-lang/rust/pull/159584/changes#r3838787968 I did indeed mean "identity macro" (cf. first part of #154045's description).
|
@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.
Add default to `TypeFoldable::fold_with`
|
The job Click to see the possible cause of the failure (guessed by this bot)Important For more information how to resolve CI failures of this job, visit this link. |
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (ca257ee): comparison URL. Overall result: ❌✅ regressions and improvements - please read:Benchmarking 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. Next, please: If you can, justify the regressions found in this try perf run in writing along with @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 3.3%, secondary -2.0%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesThis perf run didn't have relevant results for this metric. Binary sizeResults (secondary -0.0%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Bootstrap: 474.247s -> 477.756s (0.74%) |
View all comments
TypeFoldable::fold_withdelegates toTypeFoldable::try_fold_withwith an infallible error type.