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.

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

New offsetof expression #7589

Merged
merged 8 commits into from Apr 2, 2019

Conversation

@malte-v
Copy link
Contributor

commented Mar 25, 2019

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

@malte-v malte-v referenced this pull request Mar 25, 2019

Closed

New offsetof() expression #7566

@straight-shoota
Copy link
Member

left a comment

Awesome pull request!

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

This comment has been minimized.

Copy link
@straight-shoota

straight-shoota Mar 26, 2019

Member

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

Show resolved Hide resolved 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

This comment has been minimized.

Copy link
@straight-shoota

straight-shoota Mar 26, 2019

Member

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

Show resolved Hide resolved ...tools/playground/public/vendor/codemirror-5.38.0/mode/crystal/crystal.js Outdated
Show resolved Hide resolved src/compiler/crystal/macros.cr Outdated

@malte-v malte-v force-pushed the malte-v:offsetof-expression branch from 5860357 to 89aa7cb Mar 26, 2019

malte-v and others added some commits Mar 26, 2019

@malte-v malte-v force-pushed the malte-v:offsetof-expression branch from 0e7c586 to 6c29df5 Mar 26, 2019

@malte-v

This comment has been minimized.

Copy link
Contributor Author

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
Copy link
Member

left a comment

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

@malte-v

This comment has been minimized.

Copy link
Contributor Author

commented Mar 26, 2019

@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.


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

This comment has been minimized.

Copy link
@asterite

asterite Mar 27, 2019

Member

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)

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

This comment has been minimized.

Copy link
@asterite

asterite Mar 27, 2019

Member

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
Copy link
Member

left a comment

Approved with some minor suggestions. Looks great, thanks!

@malte-v malte-v force-pushed the malte-v:offsetof-expression branch from ac43880 to ba04e09 Mar 27, 2019

@malte-v malte-v force-pushed the malte-v:offsetof-expression branch from ba04e09 to a8a502b Mar 27, 2019

@asterite asterite merged commit aa4918e into crystal-lang:master Apr 2, 2019

5 checks passed

ci/circleci: check_format Your tests passed on CircleCI!
Details
ci/circleci: test_darwin Your tests passed on CircleCI!
Details
ci/circleci: test_linux Your tests passed on CircleCI!
Details
ci/circleci: test_linux32 Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@malte-v malte-v deleted the malte-v:offsetof-expression branch Apr 5, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.