-
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
Clarify BitPack
is not for creating custom encodings
#2575
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.
Great to have it mentioned now. I only have some minor points:
-
"not the way" is a little bit weak formulation, because it still suggests that it's fine to define custom instances overall. I would be more strict and use something link "You should never define a custom instance yourself, but instead use ... . The purpose of
BitPack
is to provide uniform conversion methods only.". I also liked your explanation that "It allows no creativity." of ILA ignores custom BitPack instances #2574. -
I also would mention that
deriving (BitPack)
is fine too. At the moment it may be missinterpreted that defining a custom representation is the only way. -
Rather than dictate what the bit representation in HDL is, a BitPack instance needs to exactly match that bit representation.
I also got confused by that sentence, probably because its not immediately clear what "that bit representation" refers to.
I hoped it was clear that "that representation" in the second sentence fragment refers to "the representation" in the first sentence fragment, but would you rather see it repeated verbatim? Rather than dictate what the bit representation in HDL is, a BitPack instance needs to exactly match the bit representation in HDL. |
It still confuses me. Maybe, because "the bit representation" is just very generic: so maybe elaborating on what bit representation is meant? I mean: I get what you wanna say with the sentence, but I have to read it at least twice and need to parse it carfully to get the meaning. |
I'll rephrase entirely. |
c7a7572
to
ddafe9c
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.
LGTM now.
8a61281
to
e3e6281
Compare
@mergify rebase |
This comment was marked as off-topic.
This comment was marked as off-topic.
e3e6281
to
26cbbe5
Compare
It's easy to misconstrue the purpose of
BitPack
Still TODO:
Write a changelog entry (see changelog/README.md)