dbSta: Add -module option to report_cell_usage #6562
Conversation
| set modinst [[ord::get_db_block] findModInst $args] | ||
| if { $modinst == "NULL" } { | ||
| sta_error 1002 "Unable to find $args" | ||
| if { [info exists keys(-module)] } { |
There was a problem hiding this comment.
This change does not appear to be backwards compatible, can you please add a test or revert one of the tests to ensure backwards compatibility
There was a problem hiding this comment.
It is true that this is not backward compatible. When I was discussing with @maliberty in a previous pull request, he suggested introducing such an option and backward compatibility did not seem to be a concern. See #6469 (comment)
There was a problem hiding this comment.
Since I rely on the current API I would prefer maintaining backwards compatibility (you can add a deprecation warning if you would like)
There was a problem hiding this comment.
How can I add a new option while keeping backward compatibility? For example, if I want to add "-format" and "-file" options, is it doable while keeping backward compatibility? Currently the command assumes there is only one option (not flag) that refers to the target module.
There was a problem hiding this comment.
The flags are handled separately from the positional arguments, so adding new arguments doesn't have to change anything else.
There was a problem hiding this comment.
I think you might misunderstand my comment. I am saying that the module seems to be an option to me, and I want to add two more. It would be weird that some options have keys while some do not...
|
@maliberty can you help chime in here? It would be good if you and @gadfort can reach a consensus before I make further changes. |
Signed-off-by: Bangqi Xu <[email protected]>
Have you got a chance to discuss this? Thanks. |
|
Had discussion with @QuantamHD and I didn't know that $args is whatever is not matched. I will add "-module" then. |
It sounds like you have a next step then. Let me know when you are ready for a next review. Btw, you'll need to fix the DCO before this can be merged. |
|
clang-tidy review says "All clean, LGTM! 👍" |
Will do. Thank you! |
As discussed before, we will introduce another few options to report_cell_usage. As a prerequisite, we need to add a "-module" option to the current report_cell_usage.