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

Strip Object and Outer Class from desiredName #1194

Merged
merged 1 commit into from
Oct 7, 2019
Merged

Conversation

seldridge
Copy link
Member

@seldridge seldridge commented Sep 27, 2019

This implements the fix proposed in #1166.

In effect, this strips package names, object names, and outer classes from inner classes when computing the desiredName. This mirrors the behavior of what I think desiredName is supposed to be, i.e., behaving like getSimpleName.

An effect of this is that naming behaves much better in the REPL.

Related issue: Fixes #1166

Type of change: other enhancement

Impact: API modification

Development Phase: implementation

@seldridge seldridge requested a review from a team as a code owner September 27, 2019 19:52
@seldridge seldridge changed the title Issue 1166 Strip Object and Outer Class from desiredName Sep 27, 2019
Copy link
Contributor

@chick chick left a comment

Choose a reason for hiding this comment

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

This looks good to me and it makes sense.
Only question is: Does this count as an API change?

@seldridge
Copy link
Member Author

It's an API change only if we count changes to generated FIRRTL (in any form) as an API change. I think that could go either way.

@jackkoenig
Copy link
Contributor

I would definitely count changed Verilog module names as API changes since that's a pretty important API

@seldridge
Copy link
Member Author

I agree with you about the API change, but I'd argue that counting on Verilog names is wrong due to problems with composition (the name can change depending on the namespace even with desiredName overridden).

We probably need some way of failing to elaborate if a desired name isn't successfully set as I don't think there's a mechanism to guarantee a name.

@seldridge
Copy link
Member Author

@jackkoenig: What's the merge policy for this? Would this need to run through additional regressions given the fact that it's changing Verilog name generation?

Also, @jackkoenig: this should enable us to use custom mdoc tags on the website to generate FIRRTL and Verilog without resulting in terrible readiwiwiwiwiwFoo module names.

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. Can override if needed...?

@azidar azidar added this to the 3.2.0 milestone Oct 7, 2019
@seldridge seldridge merged commit 98bfe24 into master Oct 7, 2019
@albert-magyar albert-magyar deleted the issue-1166 branch May 18, 2020 04:19
jackkoenig pushed a commit that referenced this pull request Feb 28, 2023
* Add spec for Analog type and attach statement

* Describe role of attaches in partial connection algorithm
* Change references that describe ground types where appropriate
* Closes #1194

* Fix typo
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Default desiredName and Inner Classes / Objects
4 participants