Skip to content
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

Merged
merged 1 commit into from
Sep 17, 2023

Conversation

DigitalBrains1
Copy link
Member

It's easy to misconstrue the purpose of BitPack

Still TODO:

  • Write a changelog entry (see changelog/README.md)
  • Check copyright notices are up to date in edited files

Copy link
Contributor

@kleinreact kleinreact left a 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.

@DigitalBrains1
Copy link
Member Author

  • 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.

@kleinreact
Copy link
Contributor

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.

@DigitalBrains1
Copy link
Member Author

I'll rephrase entirely.

Copy link
Contributor

@kleinreact kleinreact left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM now.

@DigitalBrains1 DigitalBrains1 force-pushed the clarify-bitpack branch 2 times, most recently from 8a61281 to e3e6281 Compare September 11, 2023 15:57
@DigitalBrains1
Copy link
Member Author

@mergify rebase

@mergify

This comment was marked as off-topic.

@DigitalBrains1 DigitalBrains1 merged commit 9bfca00 into master Sep 17, 2023
13 checks passed
@DigitalBrains1 DigitalBrains1 deleted the clarify-bitpack branch September 17, 2023 18:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants