Skip to content

Conversation

@A248
Copy link
Contributor

@A248 A248 commented Jan 14, 2026

A few changes are bundled into this PR. It's sort of code cleanup, but it also removes some unneeded panics and adds valuable documentation.

  1. Switched to &raw mut and stopped creating intermediate references. This is kinda UB even though it currently doesn't flag MIRI. So best to use the pointer address syntax.
  2. I removed the undocumented panic on ZST slice elements. It was unnecessary, and I did the "thinking" about ZST types that the code was longing for.
  3. Allocating an Arc from a layout can be a single function, plus I added panic messages when the layout size is too big.
  4. Improved the documentation on public functions in a few places.

@Manishearth
Copy link
Owner

(I do plan to look at this, I'm on vacation and have a large pile of stuff to handle when I get back)

Copy link
Owner

@Manishearth Manishearth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@Manishearth Manishearth requested a review from Copilot January 20, 2026 19:26
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR modernizes pointer handling in DST (dynamically sized type) operations by adopting &raw mut syntax, removes unnecessary ZST panics, consolidates allocation logic, and enhances documentation for safer and clearer code.

Changes:

  • Replaced intermediate reference creation with &raw mut pointer operations to avoid potential UB
  • Removed unnecessary panics for zero-sized types and added logic to handle them correctly
  • Consolidated Arc allocation logic and added explicit panic messages for oversized layouts

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File Description
src/header.rs Updated pointer operations to use &raw mut, removed ZST assertions, improved documentation with panic conditions
src/arc.rs Consolidated allocation functions, replaced unwrap calls with expect messages, enhanced safety documentation

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

src/arc.rs Outdated
.pad_to_align();

// ArcInner never has a zero size
let ptr = alloc::alloc::alloc(full_layout);
Copy link

Copilot AI Jan 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extra whitespace between '=' and 'alloc'. Should be single space.

Suggested change
let ptr = alloc::alloc::alloc(full_layout);
let ptr = alloc::alloc::alloc(full_layout);

Copilot uses AI. Check for mistakes.
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably do fix this.

(I should make sure this crate has rustfmt CI, but I'll do that later)

@Manishearth
Copy link
Owner

&raw is not yet available on this crate's MSRV.

I am fine with updating MSRV to 1.82, updating the test, and landing this. 1.82 is a year and a half old.

@A248
Copy link
Contributor Author

A248 commented Jan 21, 2026

The &raw const syntax is just a shiny replacement for addr_of!, which other parts of this crate are already using.

So it's up to you - updating to 1.82 boils down to your preference as a maintainer.

I think a more important question is, given this crate's role in the ecosystem, what do you think about releasing a version 1.0 of triomphe? Could be a good idea.

@Manishearth
Copy link
Owner

CI still fails, needs to work without std

I think a more important question is, given this crate's role in the ecosystem, what do you think about releasing a version 1.0 of triomphe? Could be a good idea.

shrug. I don't consider 1.0 to be a "must reach" goal for crates like this. I think this crate is basically "done", but I also do not want to have a major release just so we can clean up the number. It would be nice to at some point go through and look for potential breaking changes that would improve this crate, and then release a 1.0.

@Manishearth Manishearth merged commit bfac68c into Manishearth:master Jan 22, 2026
6 checks passed
@Manishearth
Copy link
Owner

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants