From a1fd01107b8e23a6b12d0c16badcb96ebc88aab6 Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Thu, 26 Sep 2024 11:08:46 +0200 Subject: [PATCH] EntryWriter: Add comments for why ManuallyDrop is OK (#380) * EntryWriter: Add comments for why ManuallyDrop is OK This wasn't immediately obvious to me at least. xref: https://github.com/alexcrichton/tar-rs/pull/376#discussion_r1774321907 Signed-off-by: Colin Walters Co-authored-by: xzfc <5121426+xzfc@users.noreply.github.com> --- src/builder.rs | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/src/builder.rs b/src/builder.rs index 3c943591..b9baaedd 100644 --- a/src/builder.rs +++ b/src/builder.rs @@ -502,6 +502,8 @@ impl SeekWrite for T { /// After writing all data to the entry, it must be finalized either by /// explicitly calling [`EntryWriter::finish`] or by letting it drop. pub struct EntryWriter<'a> { + // NOTE: Do not add any fields here which require Drop! + // See the comment below in finish(). obj: &'a mut dyn SeekWrite, header: &'a mut Header, written: u64, @@ -527,6 +529,15 @@ impl EntryWriter<'_> { /// Finish writing the current entry in the archive. pub fn finish(self) -> io::Result<()> { + // NOTE: This is an optimization for "fallible destructuring". + // We want finish() to return an error, but we also need to invoke + // cleanup in our Drop handler, which will run unconditionally + // and try to do the same work. + // By using ManuallyDrop, we suppress that drop. However, this would + // be a memory leak if we ever had any struct members which required + // Drop - which we don't right now. + // But if we ever gain one, we will need to change to use e.g. Option<> + // around some of the fields or have a `bool finished` etc. let mut this = std::mem::ManuallyDrop::new(self); this.do_finish() }