-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Do not hardcode ReferenceStorage as a built-in type
#14204
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
|
|
@@ -41,13 +41,29 @@ class Crystal::TopLevelVisitor < Crystal::SemanticVisitor | |||||||
|
|
||||||||
| @last_doc : String? | ||||||||
|
|
||||||||
| # special types recognized for `@[Primitive]` | ||||||||
| private enum PrimitiveType | ||||||||
| ReferenceStorageType | ||||||||
| end | ||||||||
|
|
||||||||
| def visit(node : ClassDef) | ||||||||
| check_outside_exp node, "declare class" | ||||||||
|
|
||||||||
| scope, name, type = lookup_type_def(node) | ||||||||
|
|
||||||||
| annotations = read_annotations | ||||||||
|
|
||||||||
| special_type = nil | ||||||||
| process_annotations(annotations) do |annotation_type, ann| | ||||||||
| case annotation_type | ||||||||
| when @program.primitive_annotation | ||||||||
| arg = ann.args.first.as(SymbolLiteral) | ||||||||
| unless special_type = PrimitiveType.parse?(arg.value) | ||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nitpick(style): I found it hard to recognize that this line assigns to
Suggested change
A trailing |
||||||||
| arg.raise "BUG: Unknown primitive type #{arg.value.inspect}" | ||||||||
| end | ||||||||
| end | ||||||||
| end | ||||||||
|
|
||||||||
| created_new_type = false | ||||||||
|
|
||||||||
| if type | ||||||||
|
|
@@ -70,14 +86,30 @@ class Crystal::TopLevelVisitor < Crystal::SemanticVisitor | |||||||
| end | ||||||||
| else | ||||||||
| created_new_type = true | ||||||||
| if type_vars = node.type_vars | ||||||||
| type = GenericClassType.new @program, scope, name, nil, type_vars, false | ||||||||
| type.splat_index = node.splat_index | ||||||||
| else | ||||||||
| type = NonGenericClassType.new @program, scope, name, nil, false | ||||||||
| case special_type | ||||||||
| in Nil | ||||||||
| if type_vars = node.type_vars | ||||||||
| type = GenericClassType.new @program, scope, name, nil, type_vars, false | ||||||||
| type.splat_index = node.splat_index | ||||||||
| else | ||||||||
| type = NonGenericClassType.new @program, scope, name, nil, false | ||||||||
| end | ||||||||
| type.abstract = node.abstract? | ||||||||
| type.struct = node.struct? | ||||||||
| in .reference_storage_type? | ||||||||
| unless type_vars = node.type_vars | ||||||||
| node.raise "BUG: Expected reference_storage_type to be a generic struct" | ||||||||
| end | ||||||||
| unless type_vars.size == 1 | ||||||||
| node.raise "BUG: Expected reference_storage_type to have a single generic type parameter" | ||||||||
| end | ||||||||
| 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 | ||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. chore: Can we also validate |
||||||||
| type.declare_instance_var("@type_id", @program.int32) | ||||||||
| type.can_be_stored = false | ||||||||
| end | ||||||||
| type.abstract = node.abstract? | ||||||||
| type.struct = node.struct? | ||||||||
| end | ||||||||
|
|
||||||||
| type.private = true if node.visibility.private? | ||||||||
|
|
@@ -133,6 +165,10 @@ class Crystal::TopLevelVisitor < Crystal::SemanticVisitor | |||||||
| if superclass.struct? && !superclass.abstract? | ||||||||
| node.raise "can't extend non-abstract struct #{superclass}" | ||||||||
| end | ||||||||
|
|
||||||||
| if type.is_a?(GenericReferenceStorageType) && superclass != @program.value | ||||||||
| node.raise "BUG: Expected reference_storage_type to inherit from Value" | ||||||||
| end | ||||||||
| end | ||||||||
|
|
||||||||
| if created_new_type | ||||||||
|
|
@@ -375,7 +411,7 @@ class Crystal::TopLevelVisitor < Crystal::SemanticVisitor | |||||||
|
|
||||||||
| process_def_annotations(node, annotations) do |annotation_type, ann| | ||||||||
| if annotation_type == @program.primitive_annotation | ||||||||
| process_primitive_annotation(node, ann) | ||||||||
| process_def_primitive_annotation(node, ann) | ||||||||
| end | ||||||||
|
|
||||||||
| node.add_annotation(annotation_type, ann) | ||||||||
|
|
@@ -460,17 +496,8 @@ class Crystal::TopLevelVisitor < Crystal::SemanticVisitor | |||||||
| false | ||||||||
| end | ||||||||
|
|
||||||||
| private def process_primitive_annotation(node, ann) | ||||||||
| 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 | ||||||||
|
|
||||||||
| value = arg.value | ||||||||
| private def process_def_primitive_annotation(node, ann) | ||||||||
| value = ann.args.first.as(SymbolLiteral).value | ||||||||
|
|
||||||||
| primitive = Primitive.new(value) | ||||||||
| primitive.location = node.location | ||||||||
|
|
@@ -924,7 +951,7 @@ class Crystal::TopLevelVisitor < Crystal::SemanticVisitor | |||||||
| if annotation_type == @program.call_convention_annotation | ||||||||
| call_convention = parse_call_convention(ann, call_convention) | ||||||||
| elsif annotation_type == @program.primitive_annotation | ||||||||
| process_primitive_annotation(external, ann) | ||||||||
| process_def_primitive_annotation(external, ann) | ||||||||
| else | ||||||||
| ann.raise "funs can only be annotated with: NoInline, AlwaysInline, Naked, ReturnsTwice, Raises, CallConvention" | ||||||||
| end | ||||||||
|
|
||||||||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -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)] | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. suggestion: Use camel case for primitive types, equivalent to type names.
Suggested change
This is a minor thing. Either spelling should work because it's backed by |
||||||
| struct ReferenceStorage(T) < Value | ||||||
| private def initialize | ||||||
| end | ||||||
|
|
||||||
|
|
||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: Is this an integral part of this patch? It looks like a tangential issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah this would probably prevent us from "exploiting"
@[Primitive]in other places in a backward-compatible manner