Add TODOs related to #429 and quotes to safety comments.

[samuelselleck]

While reading through some of the code I added some quotes and TODOs here and there related to #429.

[joshlf]

Awesome, thank you so much for these! This is exactly what we're looking for. A few minor style nits, but the content is good.

[samuelselleck]

Thank you for your feedback! Don't worry about being picky, that's the kind of thing I'm looking to learn :)

I think I've addressed all of your comments, if there is anything else do let me know.

Question: The code still contains safety comments (starting with //SAFETY:) that are not wrapped in the safety_comment! macro. Is this intentional and if so what is the purpose of the differentiation?

(I had some git problems, but think they are fixed? (accidentally merged upstream changes through the github gui). If anything is off do let me know and I'll try to figure it out.)

[joshlf]

I think I've addressed all of your comments, if there is anything else do let me know.

Awesome, thanks! I'll try to get to these soon.

Question: The code still contains safety comments (starting with //SAFETY:) that are not wrapped in the safety_comment! macro. Is this intentional and if so what is the purpose of the differentiation?

Yeah, that's intentional. Normally you'd just put a // SAFETY: comment on each unsafe block. The reason for the safety_comment! macro is that we often have a lot of unsafe trait impls whose soundness is based on identical or very similar arguments, and so we'd like to be able to write a single // SAFETY: comment to annotate multiple such impls. Since there's no way to do this that is legible to Clippy, we use the safety_comment! macro instead, which under the hood just annotates each macro invocation with #[allow(clippy::undocumented_unsafe_blocks)]:

zerocopy/src/macros.rs

Lines 24 to 29 in 9ce05f8

	macro_rules! safety_comment {
	(#[doc = r" SAFETY:"] $($(#[doc = $_doc:literal])* $macro:ident!$args:tt;)*) => {
	#[allow(clippy::undocumented_unsafe_blocks)]
	const _: () = { $($macro!$args;)* };
	}
	}

(I had some git problems, but think they are fixed? (accidentally merged upstream changes through the github gui). If anything is off do let me know and I'll try to figure it out.)

Seems fine to me 🤷‍♂️ We have this repo configured to squash all commits into a single one when a PR is merged, so it probably doesn't make much of a difference (perhaps with the exception that the resulting commit message will include a summary of each commit that was included in the PR). Personally my flow is to always just edit the commit and then force push, but whatever flow works for you is fine.

[samuelselleck]

Thanks for the explanation of the safety_comment macro, that makes sense! I'll adopt the same push -f method for edits in the future :)

[joshlf]

Okay, this looks great! Just a few tiny nits, but as soon as these are resolved, I'll approve it 🙂

[samuelselleck]

Added your changes! I'll make sure to keep spacing and use [] when quoting in the middle of a sentence. merged all commits into one now as well.

[joshlf]

Thanks again for all your work on this! I cleaned a few things up (deleting spurious extra newlines and such) and it's ready to merge.

[samuelselleck]

Apologies for not getting back to you sooner, thanks for taking the time!

[joshlf]

Apologies for not getting back to you sooner, thanks for taking the time!

No worries at all! I was just going back and cleaning up old PRs and I figured it was silly to ask you to do a few obvious edits when I was doing it already 🙂