Skip to content
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.

Sign up for GitHub

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

Jump to bottom

New offsetof expression #7589

Merged
merged 8 commits into from
Apr 2, 2019
Merged

New offsetof expression #7589

merged 8 commits into from
Apr 2, 2019

Conversation

malte-v
Copy link
Contributor

@malte-v malte-v commented Mar 25, 2019

This adds a new offsetof expression to the language, as discussed in issue #7566.

@malte-v malte-v mentioned this pull request Mar 25, 2019
Closed
straight-shoota
straight-shoota reviewed Mar 26, 2019
View reviewed changes
Copy link
Member

@straight-shoota straight-shoota left a comment

Choose a reason for hiding this comment

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

Awesome pull request!

spec/compiler/semantic/offsetof_spec.cr Outdated
assert_error "module Foo; @a = 0; end; offsetof(Foo, @a)", "Foo is neither a class nor a struct, it's a module"
end

it "errors on offsetof uninstantiated generic type" do
Copy link
Member

Choose a reason for hiding this comment

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

Maybe there should also be a successful example for offsetof(Foo(T), @a)

src/compiler/crystal/macros.cr Outdated
end

# Returns the name of the instance variable used in this `offsetof` expression as a MacroId.
def member : MacroId
Copy link
Member

Choose a reason for hiding this comment

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

We're using the name instance_variable in other places, so this should follow.

wooster0
wooster0 suggested changes Mar 26, 2019
View reviewed changes
@malte-v malte-v force-pushed the offsetof-expression branch from 5860357 to 89aa7cb Compare March 26, 2019 14:58
@malte-v malte-v force-pushed the offsetof-expression branch from 0e7c586 to 6c29df5 Compare March 26, 2019 15:47
@malte-v
Copy link
Contributor Author

malte-v commented Mar 26, 2019

All specs, including the ones I just added, should be working now. I've also undone the changes to the codemirror file and renamed structure and member to offsetof_type and instance_var, respectively.

straight-shoota
straight-shoota approved these changes Mar 26, 2019
View reviewed changes
Copy link
Member

@straight-shoota straight-shoota left a comment

Choose a reason for hiding this comment

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

Looks good 👍
But I'd like to hear some thoughts about #7566 (comment)

@malte-v
Copy link
Contributor Author

malte-v commented Mar 26, 2019 •
edited
Loading

@straight-shoota Sorry, didn't look into the issue once more. I'd definitely go with the offsetof(Type, @instance_var) syntax. It resembles the C style while also showing that this only works with instance variables. Type.@instance_var looks a bit noisy and like invoking a class method to me. If you're using offsetof, you're likely to know it from C and you would know that @instance_var references some instance variable of the specified type. But that's just my opinion.

asterite
asterite reviewed Mar 27, 2019
View reviewed changes
src/compiler/crystal/semantic/main_visitor.cr Outdated

instance_vars = type.all_instance_vars
ivar_name = node.instance_var.as(InstanceVar).name
ivar_index = instance_vars.keys.index(ivar_name)
Copy link
Member

Choose a reason for hiding this comment

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

All of this can be replaced with type.index_of_instance_var(node.instance_var.as(InstanceVar).name) (extracting node.instance_var.as(InstanceVar).name to ivar_name is fine)

asterite
asterite reviewed Mar 27, 2019
View reviewed changes
src/compiler/crystal/semantic/main_visitor.cr Outdated
elsif type && type.instance_type.devirtualize.class?
expanded = NumberLiteral.new(@program.instance_offset_of(type.sizeof_type, ivar_index).to_s, :i32)
expanded.type = @program.int32
node.expanded = expanded
Copy link
Member

Choose a reason for hiding this comment

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

The only difference between these two branches is:

@program.offset_of(type.sizeof_type, ivar_index)
# and
program.instance_offset_of(type.sizeof_type, ivar_index)

we can assign that to a variable and do all of the expanded = ...; expanded.type = ...; node.expanded = ... outside of the ifs.

asterite
asterite approved these changes Mar 27, 2019
View reviewed changes
Copy link
Member

@asterite asterite left a comment

Choose a reason for hiding this comment

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

Approved with some minor suggestions. Looks great, thanks!

@malte-v malte-v force-pushed the offsetof-expression branch from ac43880 to ba04e09 Compare March 27, 2019 14:52
@malte-v malte-v force-pushed the offsetof-expression branch from ba04e09 to a8a502b Compare March 27, 2019 14:53
@asterite asterite added this to the 0.28.0 milestone Apr 2, 2019
@asterite asterite merged commit aa4918e into crystal-lang:master Apr 2, 2019
@straight-shoota straight-shoota mentioned this pull request Apr 2, 2019
Merged
@malte-v malte-v deleted the offsetof-expression branch April 5, 2019 16:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Sija Sija Sija left review comments

@bcardiff bcardiff bcardiff left review comments

@wooster0 wooster0 wooster0 requested changes

@asterite asterite asterite approved these changes

@straight-shoota straight-shoota straight-shoota approved these changes

Assignees
No one assigned
Labels
kind:feature topic:compiler:semantic topic:lang
Projects
None yet
Milestone
0.28.0
Development

Successfully merging this pull request may close these issues.
6 participants
@malte-v @Sija @asterite @bcardiff @straight-shoota @wooster0