RFC: Encode zero kind_id semantic in return type - #689
Conversation
LLVM returns id=0 for unknown attribute ids. We should encode this semantic via zero-cost Option<NonZeroU32>, which will provide .unwrap() and other convinient methods.
TheDan64
left a comment
TheDan64
left a comment
There was a problem hiding this comment.
Change looks good but tests need to be updated to pass the suite
|
This is a breaking change, is it not? What's the need for Option here? |
|
That's a good point that it's a breaking change and should have been documented as such. But we do make breaking changes from time to time since we don't have a stable API due to LLVM version changes. So I'm not necessarily opposed to the change... I do think that Option makes it clearer that the attribute doesn't exist rather than relying on implicit C semantics which an inkwell user may not be familiar with. Perhaps it should just be |
|
It seems like it'd be nicer to make the whole thing a bit more typesafe, with |
Yes, it is, but not very big. Wasn't documented because it's just RFC
Yes, this will be more usable, but will lose"zero-costness" (memory layout optimizations), but it's not a significant loss.
I agree, I'll try to play with this idea of wrappers. TBH I'd really like to distinguish Attribute::get_named_enum_kind and Context::get_kind_id, they has similar names and sometimes passing one's id to another works (sad experience...) |
3 participants
Description
LLVM returns id=0 for unknown attribute ids. This patch encode this semantic via zero-cost Option, which will provide .unwrap() and other convenient methods.
Reasoning
I'd like to have convenient .unwrap(), .ok_or() and other methods to validate that attribute exist. However, this patch is pure cosmetics, so feel free to drop it
Considerations
Context::create_enum_attributeChecklist