Skip to content

Reapply "Mark Crucible-related hook functions as #[inline(never)]"#203

Merged
RyanGlScott merged 2 commits intomasterfrom
T153-reapply-inline-never-attributes
Dec 3, 2025
Merged

Reapply "Mark Crucible-related hook functions as #[inline(never)]"#203
RyanGlScott merged 2 commits intomasterfrom
T153-reapply-inline-never-attributes

Conversation

@RyanGlScott
Copy link
Copy Markdown
Contributor

This re-applies the changes from #154, which were omitted from #200 due to an oversight. It is a bit unclear whether or not #[inline(never)] is strictly necessary here (see #153 (comment)), but better to be on the safe side.

To make it more likely that we will remember to apply this change in the future, I have started a "Notes" section in libs/Patches.md that very explicitly calls out the needs for #[inline(never)], and I have cited this note from the relevant patch descriptions. Going forward, we can use this "Notes" section to describe other aspects of patches that require more in-depth explanations that what can be afforded in the brief patch descriptions.

Comment thread libs/Patches.md Outdated
Comment thread libs/Patches.md Outdated
Comment thread libs/Patches.md Outdated
This re-applies the changes from #154, which were omitted from #200 due to an
oversight. It is a bit unclear whether or not `#[inline(never)]` is strictly
necessary here (see
#153 (comment)), but
better to be on the safe side.

To make it more likely that we will remember to apply this change in the
future, I have started a "Notes" section in `libs/Patches.md` that very
explicitly calls out the needs for `#[inline(never)]`, and I have cited this
note from the relevant patch descriptions. Going forward, we can use this
"Notes" section to describe other aspects of patches that require more in-depth
explanations that what can be afforded in the brief patch descriptions.
@RyanGlScott RyanGlScott force-pushed the T153-reapply-inline-never-attributes branch from 48089ec to 740fff4 Compare December 3, 2025 12:16
In the past, we have forgotten to reapply updates to particular patches (see
#153 (comment) for
one example). This adds a new documentation convention to `libs/Patches.md` to
make this less likely to occur in the future. Whenever we update the
implementation of an existing patch, we now add an *Update* line to the patch
description that summarizes what was updated. Then, when the overall patch is
reapplied during an upgrade to a new Rust toolchain version, we fold the
changes from each *Update* into the main patch, and then we remove the *Update*
line entirely.
@RyanGlScott
Copy link
Copy Markdown
Contributor Author

Based on @qsctr's suggestion in #153 (comment), I've added a new convention to the libs/Patches.md file where each update to the implementation of an existing patch is given its own Update line. That way, it becomes more obvious at a glance that the implementation of a patch is spread out over multiple commits (which need to be examined separately), and when the next Rust toolchain upgrade is performed, we remember to fold all of the changes into the main patch. After reapplying the main patch, we can then remove the Update lines.

This is in addition to the Notes system. Perhaps having both is a bit overkill, but I can foresee different use cases for each mechanism: not all patch updates will necessarily be nuanced enough to deserve their own Note, and it's conceivable that we may want to write standalone Notes in the future without needing a corresponding update to a patch.

@RyanGlScott RyanGlScott merged commit f96ad27 into master Dec 3, 2025
5 checks passed
@RyanGlScott RyanGlScott deleted the T153-reapply-inline-never-attributes branch December 3, 2025 21:42
@RyanGlScott RyanGlScott added the standard libraries Issues involving mir-json's modified versions of the Rust standard libraries label Jan 26, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

standard libraries Issues involving mir-json's modified versions of the Rust standard libraries

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants