Don't leak the function that is called on drop

[WaffleLapkin]

It probably wasn't causing problems anyway, but still, a // this leaks, please don't pass anything that owns memory is not sustainable.

I could implement a version which does not require Option, but it would require unsafe, at which point it's probably not worth it.

[rustbot]

r? @TaKO8Ki

(rustbot has picked a reviewer for you, use r? to override)

[WaffleLapkin]

r? @Nilstrieb

[Noratrieb]

not because it's a good idea but out of curiosity, what unsafe approach did you imagine?

[Noratrieb]

+1 on the rename

[Noratrieb]

this touches ty::tls and while I'm fairly certain that it won't affect perf let's just make sure it doesn't
r=me afterwards
@bors try @rust-timer queue

[bors]

⌛ Trying commit e2b9530 with merge 0a7230578c08af7d4c955133f51b1eb698f1a6b2...

[WaffleLapkin]

not because it's a good idea but out of curiosity, what unsafe approach did you imagine?

Basically using ManuallyDrop instead of option. drop then uses take() and disable uses another ManuallyDrop to prevent Drop from running, so that the function can be just dropped:

pub fn defer<F: FnOnce()>(f: F) -> OnDrop<F> {
    OnDrop(ManuallyDrop::new(f))
}

pub struct OnDrop<F: FnOnce()>(ManuallyDrop<F>);

impl<F: FnOnce()> OnDrop<F> {
    pub fn disable(self) {
        // Prevent `self`'s drop from running and taking/calling `self.0`
        let mut this = ManuallyDrop::new(self);
    
        // Safety:
        // `self.0` is only taken on drop and we prevented it from running.
        unsafe { ManuallyDrop::drop(&mut this.0) };
    }
}

impl<F: FnOnce()> Drop for OnDrop<F> {
    fn drop(&mut self) {
        // Safety:
        // This is the only place that takes `self.0` (except `disable` which is
        // mutually exclusive with this function);
        let f = unsafe { ManuallyDrop::take(&mut self.0) };
        f();
    }
}

https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=22588656290ac714396168de17c4622f

[Noratrieb]

ah, since disable takes self, now I see. yeah, that would be simple and not even that hard to get right, but it seems a little unnecessary. if perf is sad r=me on the unsafe impl too

[bors]

☀️ Try build successful - checks-actions
Build commit: 0a7230578c08af7d4c955133f51b1eb698f1a6b2 (0a7230578c08af7d4c955133f51b1eb698f1a6b2)

[rust-timer]

Finished benchmarking commit (0a7230578c08af7d4c955133f51b1eb698f1a6b2): comparison URL.

Overall result: ❌ regressions - no action needed

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

@bors rollup=never
@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	0.3%	[0.2%, 0.3%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	0.9%	[0.9%, 0.9%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-1.5%	[-1.5%, -1.5%]	1
All ❌✅ (primary)	-	-	0

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.2%	[-2.4%, -2.0%]	2
All ❌✅ (primary)	-	-	0

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 645.989s -> 647.159s (0.18%)

[Noratrieb]

@bors r+ rollup

[bors]

📌 Commit e2b9530 has been approved by Nilstrieb

It is now in the queue for this repository.