-
Notifications
You must be signed in to change notification settings - Fork 152
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 hex and octal BitVector parsing per issue 1772 #2505
Add hex and octal BitVector parsing per issue 1772 #2505
Conversation
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.
Looks pretty close, just a new nits. I would also add a changelog entry for this change and update the copyright notice at the top of the file.
Come to think of it, your other PR should also have a changelog and copyright notice too
Overall this is looking good! Given that the Haddock is also user documentation, I'd like to see more examples worked out. |
1dcf53e
to
5562a42
Compare
clash-lang#2505 (comment) Also adjusts comments/documentation so the precise strings are shown.
Automatically switching between hex, octal and binary based on the prefix, akin to how BinaryLiterals does it, may be a better option.
Copyright was added to BitVector.hs and some of the comments referred to functions they were based on where they should've referred to the functions themselves, and explained undefined mask digits either ambiguously or just incorrectly.
clash-lang#2505 (comment) Also adjusts comments/documentation so the precise strings are shown.
fed1136
to
2f0790a
Compare
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 coming back to this @NadiaYvette, I'd forgotten about this PR.
@martijnbastiaan I'm happy with this PR as it stands, can you make sure it makes it in time for 1.8?
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.
Last few changes, and then it looks good to me! :)
changelog/2023-07-29T21_30_46-04_00_Add_hex_and_octal_BitVector_parsing
Outdated
Show resolved
Hide resolved
I'll grapple people away from the release button until this is merged |
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 applied the suggestions, this look good to me now :)
Automatically switching between hex, octal and binary based on the prefix, akin to how BinaryLiterals does it, may be a better option. The API functions added are hLit and oLit, by analogy with bLit.
Fixes: #1772
Still TODO: