-
Notifications
You must be signed in to change notification settings - Fork 205
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
Add comments to BoolOps implementation. #1102
Merged
Merged
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
f69a94b
Add comments for functions and structs in assembly.rs.
andriyDev 67598f2
Add inline comments for functions and structs in assembly.rs.
andriyDev 30fefd0
Add comments and inline comments for functions and structs in spec.rs.
andriyDev aaff547
Add function and inline comments for op.rs.
andriyDev a30b500
Try to explain the difference between Proc and Spec.
andriyDev f599a5c
Remove remaining TODOs in assembly.rs comments.
andriyDev a201bb0
Merge branch 'main' into comments
frewsxcv File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Can you remove these TODOs?
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.
I can, but these TODOs really need an explanation. They make no sense to me. I tried to remove these defaults as well and tests started to fail, so clearly they are necessary but I don't have a logical reason how.
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.
@rmanoka Do you recall why this
.unwrap_or(0)
is here?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.
I don't think the number here matters. If the expression is not
Some()
, then the "snake" will complete to an exterior ring, which doesn't need a parent idx. I tried verifying by setting random numbers inside.unwrap_or()
, and tests seem to pass.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.
@andriyDev Are you OK with this explanation?
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.
Yes, sorry, I'd lost track of this. I will update the comment tonight.
As an aside, I'd much prefer this case to be handled another way (if the id is unused we shouldn't need to unwrap it). But that's not the point of this PR, that kind of change should be another PR.
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.
I've added the explanation to the comments. I believe this is ready to go then (unless there are other concerns). :)