-
Notifications
You must be signed in to change notification settings - Fork 91
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
gotypes: provide access to component address #149
base: master
Are you sure you want to change the base?
Conversation
@klauspost @rleiwang Review would be appreciated if you have time 😄 |
Codecov Report
@@ Coverage Diff @@
## master #149 +/- ##
=======================================
Coverage 78.04% 78.04%
=======================================
Files 55 55
Lines 33395 33395
=======================================
Hits 26063 26063
Misses 7185 7185
Partials 147 147
Continue to review full report at Codecov.
|
Oh joy, we seem to have hit a linter error:
https://github.com/mmcloughlin/avo/pull/149/checks?check_run_id=701016645#step:7:696 |
Filed golang/go#39220. I'll probably just change the test case to a simpler 64-bit return value to avoid this linter problem. |
Another problem:
I can't quite figure out why this is yet. Perhaps it does not handle composite types properly. Skipping |
@klauspost I have to admit I don't like this very much. I quite like the way errors right now are presented similarly to the way compilers work: multiple errors presented with a file:line reference for each. That's much cleaner than a Let me know if you have ideas on how we could avoid the The only idea I have is to drop the requirement that https://github.com/mmcloughlin/avo/blob/fa88270b07e447bf4d3d65b081c8f728bc4ba1f7/build/zmov.go This could possibly be adapted to handle larger composite types. I haven't looked at how difficult this would be yet, perhaps the move detection would be harder for composite types. Perhaps there's always a niche use case for |
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.
LGTM.
Not sure if I would call it more "unsafe" than most assembly code. But it does the job nicely!
@mmcloughlin I would <3 to have this feature. |
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.
Any way we could get this in, @mmcloughlin ?
Extends #148
Fixes #145