-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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 transforms to lint module name conflicts and override module names #2452
Conversation
Cool!
An immediate reaction... could the hash be prepended with some sort of code
that shows which algorithm it is using Maybe something like “hDEADBEEF” for
heirqrchical, or “pDEADBEEF” for ports-only, or something? Would be curious
to see that in the report at least to understand how names were resolved
for each
|
33c386a
to
c766fd9
Compare
good idea. pushed an update |
I am not sure I understand the outputs you presented. It says What are you comparing it to? When it says What is original? |
|
b51f8ba
to
1ef4050
Compare
anyone know how to fix |
Uh oh maybe my sudo = false change was premature
|
maybe split up the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, didn't realize this review was still sitting in "pending" state
Also, can you update the PR Title/description at the top to reflect the new scope of this? |
Co-authored-by: Megan Wachs <megan@sifive.com>
Co-authored-by: Megan Wachs <megan@sifive.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking better :) I think we should get rid of the "strategy" concept entirely as its just distracting now.
Great! I think removing the strategy stuff made the PR a lot cleaner. I'm happy to approve except for two minor nits (update recommendedFix and add a comment to the optionalPrereqs). I think Megan has a few unresolved comments about the tests, so once those are addressed happy to merge! |
@azidar do you need to re-review given your comments? Merging is blocked on your "request changes". I am reviewing again now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
8b9c80d
to
e6a6a88
Compare
force-pushed because rerun workflow button doesn't seem to work for me |
This PR adds the
LintConflictingModuleNames
lint rule that will check for module name collisions and theRenameDesiredNames
transform to override module names.LintConflictingModuleNames
works by looking atDesiredNameAnnotation
s and checking that all desired names target exactly one module. e.g. if two different modules are annotated withQueue
desired name this will result in a lint violation.RenameModuleNames
works by looking forOverrideDesiredNameAnnotation
s and renaming modules and their associatedDesiredNameAnnotation
s to the name override. If the same module name override targets more than one module then the name override is ignored and the targeted modules are not renamed.also adds
RenameModulesAspect
that can be used to emit name overrides and aLintConflictingModuleNamesAspect
to collectDesiredNameAnnotation
s to be checked by the lint pass.Related issue: chipsalliance/firrtl#1274
Type of change: feature request
Impact: API addition (no impact on existing code)
Development Phase: implementation
Release Notes
add
StabilizeModuleNames
transform