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

Add ports #403

Closed
wants to merge 5 commits into from
Closed

Add ports #403

wants to merge 5 commits into from

Conversation

rsetaluri
Copy link
Collaborator

TODO: add tests

@coveralls
Copy link

coveralls commented May 24, 2019

Pull Request Test Coverage Report for Build 1811

  • 30 of 31 (96.77%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.09%) to 73.031%

Changes Missing Coverage Covered Lines Changed/Added Lines %
magma/circuit.py 13 14 92.86%
Totals Coverage Status
Change from base Build 1796: 0.09%
Covered Lines: 4154
Relevant Lines: 5688

💛 - Coveralls

Copy link
Collaborator

@leonardt leonardt left a comment

Choose a reason for hiding this comment

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

LGTM, the only concern I have would be that this keeps instance objects around indefinitely so long as the class is alive because it maintains a reference. I think Python's weakref would allow us to mitigate this issue: https://docs.python.org/3/library/weakref.html

tests/test_editing.py Show resolved Hide resolved
@leonardt
Copy link
Collaborator

@rsetaluri what do you think about improving the logic to use weakref? I thought about it more, and this is mainly an issue when we add the notion of deleting instances (because then the class will hold the remaining reference), but in theory it could still manifest in the current code when a Circuit object goes out of scope (it will get destructed, releasing it's reference to it's placed instances, however those placed instances won't get cleaned up because their definition class still maintains a reference to them).

@rsetaluri
Copy link
Collaborator Author

This is stale. We will revisit this feature in the future properly.

@rsetaluri rsetaluri closed this Dec 10, 2019
@rsetaluri rsetaluri deleted the add-ports branch December 10, 2019 04:53
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.

None yet

3 participants