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

hierarchy: Increase ScopeRef size #24

Merged
merged 1 commit into from
Sep 24, 2024
Merged

hierarchy: Increase ScopeRef size #24

merged 1 commit into from
Sep 24, 2024

Conversation

hovind
Copy link
Contributor

@hovind hovind commented Sep 24, 2024

I run into some issues with

let wrapped_id = HierarchyItemId::Scope(ScopeRef::from_index(node_id).unwrap());
panic()-ing on VCDs generated by gatelist simulations.

I don't know if that's a use case that you want to support, but I figured I should share my experience either way. Thanks for the great library!

@ekiwi
Copy link
Owner

ekiwi commented Sep 24, 2024

Thanks! This seems very reasonable. I am often a bit too conservative.

@ekiwi ekiwi merged commit 73d2294 into ekiwi:main Sep 24, 2024
9 of 12 checks passed
@ekiwi
Copy link
Owner

ekiwi commented Sep 24, 2024

This enhancement is part of v0.11.1 which I just pushed to crates: https://crates.io/crates/wellen
Thanks!

@hovind
Copy link
Contributor Author

hovind commented Sep 24, 2024

This enhancement is part of v0.11.1 which I just pushed to crates: https://crates.io/crates/wellen Thanks!

Thanks for looking at this so quickly. It seems I failed to consider test_sizes() in hierarchy.rs, pushed a proposal to b3afbdd, but I defer to your judgement.

@ekiwi
Copy link
Owner

ekiwi commented Sep 24, 2024

It seems I failed to consider test_sizes() in hierarchy.rs, pushed a proposal to b3afbdd, but I defer to your judgement.

Those tests are extremely brittle, so no worries about missing them. I merged your commit and then added a fix for the tests on top of it before publishing the new version:
2cb866d

@ekiwi
Copy link
Owner

ekiwi commented Sep 24, 2024

Are you using wellen as part of Surfer? If so, you might want to make a PR on the Surfer repo to bump the underlying wellen version.

@hovind
Copy link
Contributor Author

hovind commented Sep 25, 2024

Are you using wellen as part of Surfer? If so, you might want to make a PR on the Surfer repo to bump the underlying wellen version.

I do indeed, good idea! Brushed some dust off my Gitlab account: https://gitlab.com/surfer-project/surfer/-/merge_requests/480.

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