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 .toRelativeTargetToHierarchy #4067

Merged
merged 7 commits into from
May 22, 2024
Merged

Add .toRelativeTargetToHierarchy #4067

merged 7 commits into from
May 22, 2024

Conversation

mwachs5
Copy link
Contributor

@mwachs5 mwachs5 commented May 14, 2024

Contributor Checklist

  • Did you add Scaladoc to every public function/method?
  • Did you add at least one test demonstrating the PR?
  • Did you delete any extraneous printlns/debugging code?
  • Did you specify the type of improvement?
  • Did you add appropriate documentation in docs/src?
  • Did you request a desired merge strategy?
  • Did you add text to be included in the Release Notes for this change?

Type of Improvement

  • Feature (or new API)

Desired Merge Strategy

  • Squash: The PR will be squashed and merged (choose this if you have no preference).

Release Notes

Add .toRelativeTargetToHierarchy for getting .toRelativeTarget functionality when the root is a Definition or Instance.

Reviewer Checklist (only modified by reviewer)

  • Did you add the appropriate labels? (Select the most appropriate one based on the "Type of Improvement")
  • Did you mark the proper milestone (Bug fix: 3.6.x, 5.x, or 6.x depending on impact, API modification or big change: 7.0)?
  • Did you review?
  • Did you check whether all relevant Contributor checkboxes have been checked?
  • Did you do one of the following when ready to merge:
    • Squash: You/ the contributor Enable auto-merge (squash), clean up the commit message, and label with Please Merge.
    • Merge: Ensure that contributor has cleaned up their commit history, then merge with Create a merge commit.

@mwachs5 mwachs5 requested a review from azidar May 14, 2024 17:24
@mwachs5 mwachs5 added the Feature New feature, will be included in release notes label May 14, 2024
@mwachs5 mwachs5 changed the title toRelativeTarget for Hierarchy [WIP] Add .toRelativeTargetToHierarchy May 22, 2024
@mwachs5 mwachs5 marked this pull request as ready for review May 22, 2024 10:12
@mwachs5 mwachs5 requested a review from mikeurbach May 22, 2024 10:12
Copy link
Contributor

@mikeurbach mikeurbach left a comment

Choose a reason for hiding this comment

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

Thanks for taking this up. I think this is what we wanted here, because I started trying to detect if I was "inside a Definition" from the existing .toRelativeTarget API and that wasn't even really a well formed question. I'll take a deeper look at this but in general +1 from me.

Copy link
Contributor

@azidar azidar left a comment

Choose a reason for hiding this comment

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

LGTM!

*/
def toRelativeTarget(root: Option[BaseModule]): IsModule = d.proto.toRelativeTarget(root)

/** If this is an instance of a Module, returns the toAbsoluteTarget of this instance
Copy link
Contributor Author

Choose a reason for hiding this comment

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

by the way @mikeurbach @azidar are these scaladoc comments intentionally wrong? I guess a toRelativeTarget for a definition is always going to be an absoluteTarget, but is this just a copy-paste weirdness that I am perpetuating

Copy link
Contributor

Choose a reason for hiding this comment

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

I think these comments have been here since before I started contributing to Chisel, but maybe we can at least update the new ones?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i will clean these up more comprehenstively in a follow-up

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh wait I added these comments 😂 sorry for the noise.

Copy link
Contributor

@mikeurbach mikeurbach left a comment

Choose a reason for hiding this comment

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

I think this makes sense, definitely seems better than the hacky thing I was trying as evidenced by the various positive/negative test scenarios.

@mwachs5 mwachs5 added this to the 7.0 milestone May 22, 2024
@mwachs5 mwachs5 merged commit 7e0cd10 into main May 22, 2024
15 checks passed
@mwachs5 mwachs5 deleted the relative-target-hierarchy branch May 22, 2024 17:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature New feature, will be included in release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants