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

Struct casting not supported by LLVM, suggest+implement struct pointer casting #162

Closed
wants to merge 5 commits into from

Conversation

kyegupov
Copy link
Contributor

A better error message for #161, to catch the problem before the LLVM verifier does.

@kyegupov
Copy link
Contributor Author

Whoops, this needs more work: turns out struct pointers do not get casted either

@kyegupov kyegupov changed the title A warning about currently unsupported struct conversion Struct casting not supported by LLVM, suggest+implement struct pointer casting Jan 31, 2019
"converting between struct types is not yet supported; consider converting pointers as a workaround")
}
case *types.Pointer:
if _, ok := underlying.Elem().Underlying().(*types.Struct); ok {
Copy link
Member

@aykevl aykevl Jan 31, 2019

Choose a reason for hiding this comment

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

I think it's fine to always bitcast pointers. A superfluous bitcast is trivially optimized away, and it might make the code a bit simpler and more generic (think of double indirection for example).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@kyegupov kyegupov force-pushed the fix-struct-cast branch 2 times, most recently from 2c6ca26 to e532de1 Compare February 5, 2019 06:17
@deadprogram deadprogram changed the base branch from master to dev February 5, 2019 18:53
@deadprogram
Copy link
Member

Hi @kyegupov I switched this branch to point to the dev branch, which is the new active development branch. However I see that the tests are failing from your last commit.

@kyegupov
Copy link
Contributor Author

kyegupov commented Feb 6, 2019

@deadprogram done

@aykevl
Copy link
Member

aykevl commented Feb 7, 2019

@kyegupov are you planning to fully implement this feature - that is, by extracting all values from the struct and creating a new struct with the new values? If not, I can merge this PR but will likely implement that myself.
I think such splitting/joining will be optimized away entirely, as LLVM tries to break up aggregates anyway to improve other optimization passes. And if not, slow code is always better than code that fails to compile.

@kyegupov
Copy link
Contributor Author

kyegupov commented Feb 8, 2019

Sorry, I'm no longer working on the project that required tinygo, so I'm not likely to invest more effort into this.

@aykevl
Copy link
Member

aykevl commented Apr 6, 2019

Closed by #258.

@aykevl aykevl closed this Apr 6, 2019
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