global: Remove the STATIC macro. #13763
Conversation
|
@jimmo The comment script took me just under 30 minutes. Not sure if that counts as good automation or not? 😁 (Leaving hundreds of comments would take hours but I would never do that, the real time saved is PR authors knowing in advance that when they rebase they'll have to apply the change.) |
|
Code size report: |
projectgus
force-pushed
the
refactor/remove_static_macro
branch
from
290614c
to
1f57945
Compare
projectgus
force-pushed
the
refactor/remove_static_macro
branch
from
1f57945
to
a66b830
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #13763 +/- ##
==========================================
+ Coverage 98.36% 98.39% +0.02%
==========================================
Files 161 161
Lines 21084 21078 -6
==========================================
Hits 20739 20739
+ Misses 345 339 -6 ☔ View full report in Codecov by Sentry. |
@projectgus Excellent automation, would automate again. Thanks for doing this PR :) |
|
The original reason for this STATIC macro was to define it to nothing so that all static functions become global functions, and therefore visible to certain debug tools. From commit d5df6cd: Personally I have never used this feature. And with the use of LTO and heavy inline optimisation, analysing size of individual functions when they are not static is not a good representation of the size of code when fully optimised. So I don't really see any use for this STATIC macro. And it's simpler not to have the macro, ie just use One other point in favour of removing it, is that it stops bugs with |
|
It will likewise be pain for everyone with open PRs, I guess. |
|
@projectgus For the automation script I used a slightly modified version and replaced |
Reflect the changes proposed in micropython/micropython#13763.
Reflect the changes proposed in micropython/micropython#13763.
|
Thank you for posting your methodology, it helped me prepare my codebase and raise a PR against ulab - v923z/micropython-ulab#664 And a patch against AIUI all downstream code can pretty much make this change immediately. In our case (pimoroni-pico) we pretty much universally expect
Pain is good. Separates the serious contributors from the drive bys 😆 |
Pain is pain. Besides that, I expect quite a few merge conflicts, independent from how the change will be made. - If a branch is not processed according to this PR before attempting a rebase, there will be merge conflicts at the new commits when rebasing. Git cannot tell whether
There are some ways to avoid that, like:
Obviously, code that did not use the STATIC macro is not affected. |
|
According to a grep of every open PR, those affected include (129/342): Edit: I have reformatted this list into a table and drawn attention to PRs from stakeholders in this thread.
This was achieved by fetching a .patch file for every open PR and running: Then wrangling the result into a list of URLs. |
|
That's quie a number, even if some of them can be considered as unmaintained or duplicates. |
I concur! Earliest is from January 2018!! Is there a point at which PRs should be considered stale and closed for sanity's sake? But either way, the potential impact is significant 😬 |
That would be a lot of work, looking into each of them. Damien closes sometimes some. And obviously, when they address an issue which is fixed otherwise. There could be a kind of timeout. Usually after a while there are changes to the main line which makes simple rebase impossible. If then the PR is not updated, it cannot be merged. So a check for the last change date could be a means. |
The discussion is not about waiting. It's just about the side effects and ways to proceed. Of course, who ever makes a PR has to maintain it. |
Reflect the changes proposed in micropython/micropython#13763.
projectgus
force-pushed
the
refactor/remove_static_macro
branch
from
a66b830
to
a61bc35
Compare
|
Rebased, checked no new STATIC snuck in. @Gadgetoid Thanks for running through that and generating the PR table. I am planning to do something similar by automatically commenting on these PRs after merge (script linked in description), but it is good to provide the early link back notification as well! 🙏 Encouraging to see so much proactive removal already! |
After making the global replace, did you have to manually revert some of them? |
|
Have run the reminder script, as evidenced by the many link backs above. 😅 |
Thanks! Let's see how this goes... |
|
At least that was quite a notification Tsunami. |
Reflect the changes proposed in micropython/micropython#13763.
Reflect the changes proposed in micropython/micropython#13763.
Signed-off-by: Rick Sorensen <rick.sorensen@gmail.com>
See 'global: Remove the STATIC macro.': micropython#13763 Signed-off-by: Yasmin Bosch <development@yasmin-bosch.de>
Reflect the changes proposed in micropython/micropython#13763.
Reflect the changes proposed in micropython/micropython#13763.
Reflect the changes proposed in micropython/micropython#13763.
Reflect the changes proposed in micropython/micropython#13763.
Reflect the changes proposed in micropython/micropython#13763.
Reflect the changes proposed in micropython/micropython#13763.
@dpgeorge has said for a while that he wants to get rid of the
STATICmacro. No time like the present!The macro is fully removed rather than deprecated. I think this is reasonable as it's a relatively quick refactor to clean up from, but I have a script (see below) to post a heads-up to everyone with an open PR that adds the
STATICmacro.If that's not acceptable then I think we could add a CI check that blocks addition of
STATIC, but keep the macro defined otherwise. But I think this way is probably less fiddly in some ways (doesn't leave contributors bouncing off the CI.)Methodology
git ls-files | egrep '\.[ch]$' | xargs sed -Ei "s/(^| )STATIC($| )/\1static\2/"git-grep STATIC docs/, manually fixed those casesrg -t python STATIC, manually fixed the codegen lines that used STATIC.Automated heads-up
I have knocked up a quick script to leave a comment on every PR that seems to add
STATICin the diff, letting them know that when they rebase they'll need to change it tostatic.(Of course, there will also be some inevitable merge conflicts from changed lines. There isn't really a way around that, apart from not moving away from the macro.)
Follow-up work
Separate PR georgerobotics/cyw43-driver#109 to remove STATIC from cyw43-driver repo. Once merged then can revert the commit in this branch that manually sets the macro when building those files.
This work was funded through GitHub Sponsors.