Skip to content

compiler: add support for the go keyword on interface methods #2202

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

Merged
merged 2 commits into from
Oct 31, 2021

Conversation

aykevl
Copy link
Member

@aykevl aykevl commented Oct 24, 2021

Most of this PR is a refactor of interface lowering, to make future changes possible. The result of it is that it is now trivial to add support for the go keyword on interface methods, as requested in #1961.

@aykevl
Copy link
Member Author

aykevl commented Oct 26, 2021

Updated to fix merge conflicts and (hopefully) fix the CI failures.

This commit simplifies the IR a little bit: instead of calling
pseudo-functions runtime.interfaceImplements and
runtime.interfaceMethod, real declared functions are being called that
are then defined in the interface lowering pass. This should simplify
the interaction between various transformation passes. It also reduces
the number of lines of code, which is generally a good thing.
This is a feature that was long missing, but because of the previous
refactor, it is now trivial to implement.
uintptrType := mod.Context().IntType(llvm.NewTargetData(mod.DataLayout()).PointerSize() * 8)

defer llvm.VerifyModule(mod, llvm.PrintMessageAction)
// Look up the (reflect.Value).Implements() method.
Copy link
Member

Choose a reason for hiding this comment

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

I think I am missing something: why can't we look this function up by name?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because it is named something like interface:{Align:func:{}{basic:int},AssignableTo:func:{named:reflect.Type}{basic:bool},Bits:func:{}{basic:int},Comparable:func:{}{basic:bool},ConvertibleTo:func:{named:reflect.Type}{basic:bool},Elem:func:{}{named:reflect.Type},Field:func:{basic:int}{named:reflect.StructField},FieldAlign:func:{}{basic:int},Implements:func:{named:reflect.Type}{basic:bool},Key:func:{}{named:reflect.Type},Kind:func:{}{named:reflect.Kind},Len:func:{}{basic:int},Name:func:{}{basic:string},NumField:func:{}{basic:int},NumMethod:func:{}{basic:int},Size:func:{}{basic:uintptr},String:func:{}{basic:string}}.Implements$invoke and therefore depends on the precise set of methods in src/reflect/type.go.
So yeah, we could check that name directly, but every change to the reflect.Type interface would also need a change here. Doable, but kind of a burden.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, I think I misunderstood what this was doing. Alright.

@niaow
Copy link
Member

niaow commented Oct 28, 2021

I think most of this looks fine, just a bit confused (see the question i posted above)

@deadprogram
Copy link
Member

Thanks @aykevl for working on this and thank you @niaow for review.

Now merging.

@deadprogram deadprogram merged commit 9e1b4de into dev Oct 31, 2021
@deadprogram deadprogram deleted the go-on-interface branch October 31, 2021 13:17
@deadprogram
Copy link
Member

Might have been better to rebase dev into this branch first, because after merging the CI is failing in dev branch.

@aykevl can you please take a look? Thanks!

niaow added a commit to niaow/tinygo that referenced this pull request Oct 31, 2021
kenbell pushed a commit to kenbell/tinygo that referenced this pull request Oct 31, 2021
ardnew pushed a commit to ardnew/tinygo that referenced this pull request Jun 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants