Do not hardcode ReferenceStorage as a built-in type
#14204
Conversation
HertzDevil
added
kind:bug
| when @program.primitive_annotation | ||
| # there isn't a PrimitiveAnnotation type yet, so we validate right here | ||
| if ann.args.size != 1 | ||
| ann.raise "expected Primitive annotation to have one argument" | ||
| end | ||
|
|
||
| arg = ann.args.first | ||
| unless arg.is_a?(SymbolLiteral) | ||
| arg.raise "expected Primitive argument to be a symbol literal" | ||
| end |
There was a problem hiding this comment.
question: Is this an integral part of this patch? It looks like a tangential issue.
There was a problem hiding this comment.
Yeah this would probably prevent us from "exploiting" @[Primitive] in other places in a backward-compatible manner
| @@ -13,7 +13,8 @@ | |||
| # WARNING: `ReferenceStorage` is unsuitable when instances of `T` require more | |||
| # than `instance_sizeof(T)` bytes, such as `String` and `Log::Metadata`. | |||
| @[Experimental("This type's API is still under development. Join the discussion about custom reference allocation at [#13481](https://github.com/crystal-lang/crystal/issues/13481).")] | |||
| struct ReferenceStorage(T) | |||
| @[Primitive(:reference_storage_type)] | |||
There was a problem hiding this comment.
suggestion: Use camel case for primitive types, equivalent to type names.
| @[Primitive(:reference_storage_type)] | |
| @[Primitive(:ReferenceStorageType)] |
This is a minor thing. Either spelling should work because it's backed by Enum.parse. But I think this makes a bit more sense for type names.
| case annotation_type | ||
| when @program.primitive_annotation | ||
| arg = ann.args.first.as(SymbolLiteral) | ||
| unless special_type = PrimitiveType.parse?(arg.value) |
There was a problem hiding this comment.
nitpick(style): I found it hard to recognize that this line assigns to special_type. I was looking for an assignment and didn't see this at first because it starts with an unless.
I'd suggest to split this into two lines to improve readability.
| unless special_type = PrimitiveType.parse?(arg.value) | |
| special_type = PrimitiveType.parse?(arg.value) | |
| unless special_type |
A trailing unless would also be possible.
| if node.splat_index | ||
| node.raise "BUG: Expected reference_storage_type to have no splat parameter" | ||
| end | ||
| type = GenericReferenceStorageType.new @program, scope, name, @program.value, type_vars |
There was a problem hiding this comment.
chore: Can we also validate node.struct? == true and node.abstract? == false?
abstract class ReferenceStorage(T) should be invalid.
|
Superseded by #14270 |
Fixes #14194. Checked with
make clean crystal && git checkout 1.9.0 && bin/crystal spec spec/std/int_spec.cr. (Even earlier versions would still fail due to an LLVM intrinsic name mismatch regarding opaque pointers, unrelated to this feature.)