@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:

   bare-arm:    +0 +0.000% minimal x86:    +0 +0.000%    unix x64:    +0 +0.000% standard      stm32:    +0 +0.000% PYBV10     mimxrt:    +0 +0.000% TEENSY40        rp2:    +0 +0.000% RPI_PICO       samd:    +0 +0.000% ADAFRUIT_ITSYBITSY_M4_EXPRESS

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.39%. Comparing base(90e5178) to head(decf8e6).

@@            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.
📢 Have feedback on the report?Share it here.

@jimmo The comment script took me just under 30 minutes. Not sure if that counts as good automation or not? 😁

@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 commitd5df6cd:

Some tools do not support local/static symbols (one example is GNU ld map file).Exposing all functions will allow to do detailed size comparisons, etc.

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 usestatic as-is, because then you know exactly what it's doing. Eg for newcomers they don't have to learn what the STATIC macro is.

One other point in favour of removing it, is that it stops bugs withSTATIC inline, which should always bestatic inline.

@tannewt@dhalbert Heads-up about something with potential to cause some transient pain in the next merge. :)

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
git ls-files withgit show --pretty="" --name-only <first_commit>..<last_commit> | sort | uniq, with<first_commit> and<last_commit> being the commit id's of the PR. That way, only the files are touched that belong to the PR. Otherwise all files of the repository will be processed.

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 againstpimoroni-pico -pimoroni/pimoroni-pico#906

AIUI all downstream code can pretty much make this change immediately. In our case (pimoroni-pico) we pretty much universally expectSTATIC to resolve tostatic and, afaik, have never used the mentioned technique for debugging since most of our Python bindings are just glue to underlying classes anyway.

It will likewise be pain for everyone with open PRs, I guess.

Pain is good. Separates the serious contributors from the drive bys 😆

Pain is good.

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 whetherSTATIC orstatic is right, and will eventually even include adjacent lines into the conflict's span.

• If a branch is processed according to this PR before rebase, it could only work if there are no changes to the master branch after applying the global replace. If there have been such, there will by conflicts is the base part of the code.

There are some ways to avoid that, like:

• deliberately change STATIC to static in the files changed in a branch before rebasing to the changed master. That must be done in reverse order from the newest to the oldest commit.
• create a new branch for a feature , do a bulk change of the old branch for this feature and cherry-pick the commits from the old branch to the new one.

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:

grep -lr "+STATIC" patches/ > affected.txt

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.

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 😬

Is there a point at which PRs should be considered stale and closed for sanity's sake

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.

@tannewt@dhalbert Heads-up about something with potential to cause some transient pain in the next merge. :)

No concern for me. It is an easy change to do and a win for consistency.

Waiting to do it will just lead to more code that needs to be updated. Old PRs will have to be updated either way.

Waiting to do it will just lead to more code that needs to be updated.

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.

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!

Rebased, checked no new STATIC snuck in.

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. 😅

Have run the reminder script

Thanks! Let's see how this goes...

At least that was quite a notification Tsunami.