-
Notifications
You must be signed in to change notification settings - Fork 2.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 the ability to add a diff
for the file size differ
#37764
Conversation
PR #37764: Size comparison from 9298ce7 to 091cd09 Full report (73 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
PR #37764: Size comparison from 9298ce7 to 06228b1 Increases above 0.2%:
Full report (73 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
@gemini-code-assist review |
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.
Code Review
The pull request introduces several new features and enhancements to the file size analysis tool. The addition of diffing capabilities, the icicle display type, and color options significantly improve the tool's utility and presentation. The code is generally well-structured, but there are a few areas where minor improvements can be made to enhance clarity and maintainability.
Summary of Findings
- Clarity of Symbol Diff Logic: The logic within
compute_symbol_diff
could benefit from additional comments to clarify the conditions under which symbols are considered different and how the size delta is calculated and applied. - Inconsistent use of Enum: The
ChartStyle
Enum was removed and replaced with direct references topx
functions. While this simplifies the code, it might be beneficial to maintain an Enum for clarity and potential future expansion. - Magic Numbers in Namespace Removal: The use of
separate_idx+2
to remove namespaces could be made more explicit with a named constant or a comment explaining the+2
offset.
Assessment
The pull request introduces several new features, including size differencing, an icicle display option, naming format updates, and color customization. The changes seem well-structured and the included screenshots demonstrate the functionality. However, there are a few areas where improvements can be made to enhance code clarity and maintainability. I recommend addressing the comments before merging, and ensuring that another reviewer approves this code before merging.
…p#37764) * Start implementing a diff for symbols * Fix up size computations * make things run * Added icicles graph * Also add percent root to individual items * Show name sizes * Better name formatting * Restyled by autopep8 * Make linter happy * add assert for fetch values, clean up asserts a bit * Match statements seem easier to read and provable branches with static analysis * Cleaner code by using express everywhere * Shorter code * Allow some color control too * Restyled by autopep8 --------- Co-authored-by: Andrei Litvin <andreilitvin@google.com> Co-authored-by: Restyled.io <commits@restyled.io>
Testing
Ran locally, saw display like:
and
and