[scripted-diff] No extern function declarations #12879

Declaring a function (void foo(int bar);) is default extern. The scripted commit cleans up cases of extern void foo(int bar); (with scripted diff verification) and the build commit adds a rule to the linters that forbids new code of this kind.

utACK b4264f2, nice.

Nit, 2nd commit could have same prefix format build: .

I tend to use [brackets]. I am trying to bracket the scripted-diff to see if that's accepted too.

According to @laanwj the widely used format is foo: .

Concept ACK. Thanks for adding a lint script to make this cleanup persistent!

I don't have any suggestions beyond what shellcheck suggests:

$ shellcheck contrib/devtools/lint-extern-fundecl.sh

In lint-extern-fundecl.sh line 9:
HEADER_ID_PREFIX="BITCOIN_"
^-- SC2034: HEADER_ID_PREFIX appears unused. Verify it or export it.


In lint-extern-fundecl.sh line 10:
HEADER_ID_SUFFIX="_H"
^-- SC2034: HEADER_ID_SUFFIX appears unused. Verify it or export it.


In lint-extern-fundecl.sh line 17:
    if [[ $(egrep -c "extern ([^_\"\(]+)\(" ${SOURCE_FILE}) != 0 ]]; then
            ^-- SC2196: egrep is non-standard and deprecated. Use grep -E instead.
                                            ^-- SC2086: Double quote to prevent globbing and word splitting.

Suggested changes:

@@ -6,15 +6,12 @@
 #
 # Check unnecessary extern keyword usage for function declarations.

-HEADER_ID_PREFIX="BITCOIN_"
-HEADER_ID_SUFFIX="_H"
-
 REGEXP_EXCLUDE_FILES_WITH_PREFIX="src/(crypto/ctaes/|leveldb/|secp256k1/|tinyformat.h|univalue/)"

 EXIT_CODE=0
 for SOURCE_FILE in $(git ls-files -- "*.h" "*.cpp" | grep -vE "^${REGEXP_EXCLUDE_FILES_WITH_PREFIX}")
 do
-    if [[ $(egrep -c "extern ([^_\"\(]+)\(" ${SOURCE_FILE}) != 0 ]]; then
+    if [[ $(grep -E -c "extern ([^_\"\(]+)\(" "${SOURCE_FILE}") != 0 ]]; then
         echo "${SOURCE_FILE} is using unnecessary extern keyword for function declarations"
         EXIT_CODE=1
     fi

Like the shellcheck suggestions? Please review #12871 to help bring shellcheck to Travis :-)

Not sure if this really needs a scripted diff and lint script. Seems overkill. Just remove them in one commit and mention that in the commit message.

@MarcoFalke Why remove the lint script? The lint script makes this improvement persistent. What would be good reasons to switch to a temporary fix when the permanent fix is already in place?

A temporary fix in this case will generate follow-up PRs in the future that will require review time. Better let Travis take care of finding trivial mechanical issues via the lint script so that human reviewers can focus on the important higher level stuff.

The lint script is already written, it is really easy to read and the logic is so simple that the script is very unlikely to generate false positives or any maintenance burden going forward. Looks like an obvious win to me? :-)

There is no harm in having the extern there. It is just a minor style change. With the same rationale we could start enforcing that all code is clang formatted, but that would not be maintainable. The linters are there to catch potential bugs and documentation inconsistencies, not minor style issues.

@MarcoFalke I'm not sure that is a fair comparison. A more fair comparison would be: assuming that the entire code base was 100 % clang formatted, wouldn't it then make sense to have a linter that made sure that it stayed that way and no clang formatting violations were introduced in subsequent PR:s?

Automation is our friend :-)

Adding a script of 22 lines that needs to be maintained (*) and run on every pull request is just not worth it. (Again having the redundant extern has zero harm or cost, so the trade-offs are clear, imo)

(*) See how you have already two comments for the script.

@MarcoFalke My review comments on the script just underscores my point on how automatic linting frees up review time for humans – all my review comments on this PR would all have been made automatically by shellcheck (and thus fixed before human review) once #12871 is merged.

I agree that redundant code has no "technical harm", but if automatic linting can guarantee that PR:s reaching human review are free from redundant code then I think that is an easy win.

Perhaps we differ in the assumptions we make?

Assumptions I make:

• Computing time is cheap (i.e. a few ms extra runtime in Travis doesn't matter).
• We only add linting scripts that exhibit a low rate of false positives.
• Review time is more scarce than developer time.
• We prefer that issues that humans are likely to point out during manual code review (such as Nit: I think you have a redundant extern there) to be fixed before the human review process takes place.

and run on every pull request is just not worth it @MarcoFalke IMO that should not be an argument against.

Again having the redundant extern has zero harm or cost

I'd only say "cost", because wrong indentation or code format also has zero harm.

Generally I support mechanisms to keep the code consistent. But I kind of understand @MarcoFalke points as this is not an usual case. I would totally support this if the compiler/linker had a flag to warn/error these unnecessary extern usages.

@MarcoFalke Generally, anything that could convince someone to make a PR, that could be automated, and that is repeated in new code, is worth automating. This has zero cost (and can probably even be same-binary verified), but will probably save PR time, even with the maintenance of that script (it's basically the same as the include-guards one, so if it breaks, the other one does too, most likely). @practicalswift Cool about shellchecker. Will check out the PR.

@promag That must be new (prefer foo:)! Will switch to that style. (Also did do so in these commits.)

ACK 4d3f609481b6214ff2ce4de44ed40ad66e79e630

There is no harm in having the extern there. It is just a minor style change. With the same rationale we could start enforcing that all code is clang formatted, but that would not be maintainable. The linters are there to catch potential bugs and documentation inconsistencies, not minor style issues.

I agree. Let's not over-burden the projects will all kinds of linters for trivial things. This seems over-zealous. There's no risk that a redundant 'extern' causes any actual bugs. It doesn't even cause confusion to developers reading the code.

FYI I held back on writing doc/developer-notes.md for a long time because I was afraid of things like this. Everyone has their pet issues with code style. In the end I did, but added a clear rationale for everything mentioned. The rationale should always serve to avoid bugs and vulnerabilities, as well as other problems visible to end users.

I'd also suggest to get rid of the scripted diff, it is easier to review without having to also review the scripted diff.

@laanwj

I agree. Let's not over-burden the projects will all kinds of linters for trivial things. This seems over-zealous. There's no risk that a redundant 'extern' causes any actual bugs. It doesn't even cause confusion to developers reading the code.

That's kind of the point, in a way. Using extern when you don't need to has no side effects, which means a lot of people think they have to use it. By forbidding it in new code, we free up time from reviewing other people who come in later to "fix" the problem. It's trivial to fix and perhaps even delightfully educational. @MarcoFalke

I'd also suggest to get rid of the scripted diff, it is easier to review without having to also review the scripted diff.

I thought scripted diffs were implemented to make it easier to review, as you have an anchor in code that defines what you are changing. Is that not why they exist?

Agree with @kallewoof 100 %.

I don't understand the sudden dislike for regression tests and scripted-diffs :-)

I fail to see how this regression test could be a bad thing. Automation is our friend.

I agree with kallewoof and practicalswift. Scripted diffs can guard against unintentended changes, in addition to making prs easier to review. If you only want to review part of a PR, it seems easy enough to say "utACK for code changes but I didn't review the script". I also like all the linters (though I would encourage someone would add a "make lint" command so they are easier to run locally).

utACK 4d3f609481b6214ff2ce4de44ed40ad66e79e630

Closing for now, since none of the maintainers are willing to merge it due to the future maintenance overhead of the lint script. Also, needed rebase.

If you feel strongly about this style change you can still fix the style in a new pull request. If you feel strongly about the linter script, I suggest bringing up the discussion of its usefulness when a non-grandfathered (fresh) "violation" happens. Otherwise, this discussion seems purely theoretical with no practical use.

@MarcoFalke You can't just go around closing PR's because you disagree with them personally. I'm baffled by the 'no maintainer willing to merge' comment. I always presumed maintainers merged based on community consensus and this one clearly has proponents. In fact, more pro than against.

Lastly, I'm disappointed that you don't think I am capable of judging whether a PR should be closed or not. This goes for @laanwj as well, as he has closed several PR's of mine. I have personally closed a ton of PR's (check the list and you'll see) for a variety of reasons. If you insist on closing a PR, at least give the author a heads up about it so they can at least close it themselves.

@kallewoof I assume that what @MarcoFalke means is that it looks too controversial to be merged, which seems a fair reason to close things. Just a few people in favor isn't enough when there are also good arguments being raised against.

FWIW, I'm generally in favor of automating simple things that improve consistency of the codebase, but that's hard to defend when there is disagreement whether lack of extern is an advantage or not, or whether it's worth spending time on.

@sipa I understand that. Why not simply say "I suggest you close this as it's too controversial to be merged." That at least gives me the opportunity to argue or close myself (that means I can reopen later if I have some bright idea for how to address the controversial points). I'm also not sure I agree that it's too controversial. After all, only two people have argued against it, and they are the sort of people with an open enough mind that discussion could convince them to change their opinion.

Whether it's worth spending time on or not is a bit weird. We have spent 30 comments and a PR with code and review on this. Merging means we will never do so again, for this particular case at least, whereas not merging we may have this exact same discussion with different participants in a year from now about the same issue.

@kallewoof That sort of reasoning applies everyone's pet peeve for style, and does not scale. I agree with @laanwj that this simply does not matter - extern or lack thereof doesn't convey any additional information, and does not prevent any bugs.

I also believe we should not be enforcing random style opinions just because they increase consistency. That again leads to arbitrary rules that are an extra burden for developers without clear benefits.

If there is a widespread opinion among contributors and maintainers that a particul guideline is worth the burden, then I think it should be put in the style guide - and if it is, it sounds great to enforce it.

I believe that is @MarcoFalke's point: the way to make this happen is discussing whether it should be a style guideline in the first place. After that we can have a discussion about a linter. Please don't see a PR being closed as the final step.

That all makes sense, and I think it could be stated that way without forcibly closing the PR. Closing is the equivalent of saying the author is not capable of making that decision, or that the author won't listen to reason. We go from a "peer reviewed system" to a governed one. I think that it is sometimes necessary to close PR's, but often it is not, so it should be used sparingly.

I don't think you should see it that way. It's just a "this approach is not going to work, no need to keep it open". If you would decide to change approach significantly (in this case, change it into a change to the style guide, for example), you'd still be asked to do it in a separate PR to avoid people incorrectly assuming it's a continuation of the same thing and missing it. This PR being closed doesn't affect that at all.

I think @MarcoFalke could have been clearer about the reasoning, but sometimes things that don't work just need to be closed. We have far too many PRs already.