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

Cleanup handle (Part 1) #2720

Merged
merged 24 commits into from Dec 1, 2023
Merged

Cleanup handle (Part 1) #2720

merged 24 commits into from Dec 1, 2023

Conversation

ktbarrett
Copy link
Member

@ktbarrett ktbarrett commented Sep 24, 2021

Starting some of the cleanup mentioned in https://github.com/cocotb/cocotb/wiki/Handle-Cleanup-Proposal without significantly breaking the interface. Closes #1522. Addresses some of #3548.

tests/test_cases/test_array/test_array.py Outdated Show resolved Hide resolved
cocotb/handle.py Outdated Show resolved Hide resolved
@ktbarrett
Copy link
Member Author

The recursive discovery tests don't like me deprecating iteration over unit objects like reals and ints. I could condition the recursion to only objects which are iterable: HierarchyObject and NonHierarchyIndexableObject, or maybe we want to continue supporting iteration over clearly non-iterable objects just because the VPI/VHPI doesn't bomb out?

@ktbarrett
Copy link
Member Author

ktbarrett commented Sep 24, 2021

Seems like a lot of tests worked in GHDL with ConstantObject. You couldn't tell what the type was, but if you know what the type is you could access the value correctly. It is uncertain what would have happened if you accesses the value incorrectly. "Works by happenstance". removing ConstantObject will likely break some GHDL users.

Questa and Riviera are also failing because the tests were previously counting individual bits discovered by iterating over logic array or string types. I can add those back in to the recursion, but perhaps there is a better way to iterate where invalid iterations (like over reals and ints) is not allowed? Unfortunately, the test code assumes that you can iterate safely over all handles, and while that's true, it isn't sensible for some types.

@ktbarrett ktbarrett force-pushed the cleanup-handle branch 2 times, most recently from 7316295 to 08cc5ff Compare July 20, 2023 19:03
@ktbarrett ktbarrett force-pushed the cleanup-handle branch 3 times, most recently from cd24fcd to 848bc14 Compare November 10, 2023 00:17
@ktbarrett ktbarrett changed the title Cleanup handle Cleanup handle (Part 1) Nov 10, 2023
Copy link

codecov bot commented Nov 10, 2023

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (f201c2a) 66.65% compared to head (516dc99) 66.51%.

Files Patch % Lines
src/cocotb/handle.py 97.80% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2720      +/-   ##
==========================================
- Coverage   66.65%   66.51%   -0.14%     
==========================================
  Files          48       48              
  Lines        8441     8410      -31     
  Branches     2387     2387              
==========================================
- Hits         5626     5594      -32     
  Misses       1696     1696              
- Partials     1119     1120       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ktbarrett ktbarrett force-pushed the cleanup-handle branch 2 times, most recently from 95fbb44 to 2c91d3c Compare November 14, 2023 01:53
@ktbarrett ktbarrett force-pushed the cleanup-handle branch 9 times, most recently from 4b74635 to bc4aa85 Compare November 24, 2023 04:54
@ktbarrett ktbarrett marked this pull request as ready for review November 24, 2023 04:55
@ktbarrett ktbarrett requested a review from a team November 24, 2023 05:13
src/cocotb/handle.py Outdated Show resolved Hide resolved
@@ -758,13 +699,13 @@ def value(self) -> BinaryValue:
result._set_trusted_binstr(binstr)
return result

def __int__(self):
return int(self.value)


class RealObject(ModifiableObject):
"""Specific object handle for Real signals and variables."""
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"""Specific object handle for Real signals and variables."""
"""Specific object handle for real-valued signals and variables."""

(while here)

Copy link
Member Author

@ktbarrett ktbarrett Nov 24, 2023

Choose a reason for hiding this comment

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

I'll leave this as a comment for "Part 2". I want the PRs to be digestible and there are a ton of documentation things I could change. For example, this diagram will have to change.

tests/test_cases/test_discovery/test_vhdl_indexed_name.py Outdated Show resolved Hide resolved
@ktbarrett ktbarrett force-pushed the cleanup-handle branch 2 times, most recently from 6fb5813 to 84c0339 Compare November 24, 2023 23:10
@ktbarrett ktbarrett mentioned this pull request Dec 1, 2023
22 tasks
@marlonjames marlonjames added type:cleanup cleanup or refactoring on code, documentation, or other areas category:codebase:handle relating to handles or handle types (BinaryValue) labels Dec 1, 2023
@ktbarrett ktbarrett merged commit b5012f9 into cocotb:master Dec 1, 2023
23 checks passed
@ktbarrett ktbarrett deleted the cleanup-handle branch December 1, 2023 18:56
@marlonjames marlonjames added this to the 2.0 milestone Dec 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:codebase:handle relating to handles or handle types (BinaryValue) type:cleanup cleanup or refactoring on code, documentation, or other areas
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consider changing base class of NonConstantObject
3 participants