-
-
Notifications
You must be signed in to change notification settings - Fork 411
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 by Bors] - Divide byte compiler #2425
Conversation
Codecov Report
@@ Coverage Diff @@
## main #2425 +/- ##
==========================================
+ Coverage 52.44% 52.74% +0.30%
==========================================
Files 332 343 +11
Lines 34948 34940 -8
==========================================
+ Hits 18327 18430 +103
+ Misses 16621 16510 -111
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
Thanks for starting to work on this! The current changes look very much the way I would expect. |
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
I went through |
The test that's failing is related to Rustfmt. You just need to run |
Whoops, forgot to run that. Fixed it, anything else? |
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.
Thanks for woking on this. I have some suggestions for the new code in general, but I'd be happy to merge this as is and do the changes later.
Instead of the compile*
functions being free functions that take byte_compiler: &mut ByteCompiler<'_>,
as an argument, I would suggest implementing them on the ByteCompiler
.
All of the compile*
functions probably could use some minimal documentation, but we do not have any for that right now, so that may wait.
Is it possible to implement methods on `ByteCompiler` within submodules? I was under the impression that wasn't allowed which is why I made them free functions, I could pretty easily correct that if theres something I'm missing though
|
It is definitely possible, you just need to create another |
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.
Agree with @raskad about implementing byteCompiler but apart from that LGTM
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.
Thank you for the contribution!
bors r+ |
This Pull Request is currently unfinished but will fix/close #1808 after some review and more work It changes the following: - Divides byte compiler logic into separate files I would like some review on the current code I have to know if the patterns I'm using are acceptable for the codebase, if everything looks good I will try to separate more code into different small modules to finish the work here.
Pull request successfully merged into main. Build succeeded: |
This Pull Request is currently unfinished but will fix/close #1808 after some review and more work
It changes the following:
I would like some review on the current code I have to know if the patterns I'm using are acceptable for the codebase, if everything looks good I will try to separate more code into different small modules to finish the work here.