Swaps raw pointer types with NonNull for better safety and niches#687
Swaps raw pointer types with NonNull for better safety and niches#687ErisianArchitect wants to merge 5 commits intoTheDan64:masterfrom
Conversation
|
Third times the charm, I guess. |
|
Oh, I forgot to mention, I also added a couple functions to support.
|
| #[repr(transparent)] | ||
| #[derive(Debug)] | ||
| pub struct OperandBundle<'ctx> { | ||
| bundle: Cell<LLVMOperandBundleRef>, |
There was a problem hiding this comment.
As far as I could tell, bundle did not need to be a Cell, so I changed that too.
| unsafe { *self.0.get_or_insert_with(|| LLVMCreateTargetMachineOptions()) } | ||
| unsafe { | ||
| self.0 | ||
| .get_or_insert_with(|| NonNull::new_unchecked(LLVMCreateTargetMachineOptions())) |
There was a problem hiding this comment.
This is honestly a little bit weird, but it works.
There was a problem hiding this comment.
Do you think The struct should just be initialized with LLVMCreateTargetMachineOptions? Or does it being Option make sense?
|
Oh, and one last note: I didn't apply this optimization to |
|
Looks cool, it'll take me a while to review it all |
| pub(crate) enum LLVMStringOrRaw { | ||
| Owned(LLVMString), | ||
| Borrowed(*const c_char), | ||
| Borrowed(NonNull<c_char>), |
There was a problem hiding this comment.
If LLVMString and NonNull are both niche, doesn't that mean LLVMStringOrRaw can be niche as well? Or does the enum miss out on it?
There was a problem hiding this comment.
No, unfortunately it misses out on the niche since it needs an extra value for the discriminant. It can't say this variant and this value or that variant and that value in a flattened way. But LLVMStringOrRaw will have plenty of niches anyway, 2^63, I believe.
| #[inline(always)] | ||
| pub(crate) const fn const_assert(assertion: bool) { | ||
| if !assertion { | ||
| panic!("Assertion Failed"); |
There was a problem hiding this comment.
Is it possible to be able to take a static string? Would be nice if the panic had more specific details
There was a problem hiding this comment.
Unfortunately, no, not at compile time. panic expects a format argument, and format args, but it can't perform formatting at compile time.
There was a problem hiding this comment.
The best recommendation is to include a comment with the assertion. It's marked with #[track_caller], so the error will appear at the location of the assertion.
Yeah, sorry about that, haha. I really did not expect this change to affect this many lines. |
| LLVMStringOrRaw::Owned(ref llvm_string) => llvm_string.ptr, | ||
| LLVMStringOrRaw::Borrowed(ptr) => ptr, | ||
| LLVMStringOrRaw::Owned(ref llvm_string) => llvm_string.ptr.as_ptr(), | ||
| LLVMStringOrRaw::Borrowed(ptr) => ptr.as_ptr().cast_const(), |
There was a problem hiding this comment.
"cast_const" gives me C++ ptsd 😂
2 participants
Description
Many of the types used
LLVM*Reftypes, which are type aliases for*mut LLVM*. These pointers are null-checked before creating new instances of these types, so I thought it would make sense to useNonNullinstead so that there would be better safety guarantees, and it would enable these types to have at least one niche, allowing for this condition:size_of::<T>() == size_of::<Option<T>>() && align_of::<T>() == align_of::<Option<T>>().Related Issue
Closes #686
How This Has Been Tested
I finally figured out clippy, so I have a bash function that I run that runs clippy, runs all the tests, then runs cargo fmt. So all checks should hopefully pass on the first try. Fingers crossed.
It's quite a big update. It was extremely repetitive and tedious, but I think I got everything.
Option<Breaking Changes>
I'm pretty sure there are no breaking changes.
Checklist