Remove HERE.

[rousskov]

This is a controversial change because it brings no runtime benefits,
creates significant cross-branch patch porting headaches and branch
maintenance overheads, but it also removes one source of persistent
bickering during code review and improves code consistency. Global
removal is the only practical way we can remove all HEREs -- restricting
removal of HEREs to changed-for-other-reasons lines would take forever.

The (simple) script to remove HEREs will be published separately because
it is not really needed in this source tree (which no longer uses HERE).

This pull request has two commits that should not be squashed IMO. One
automatically removes HERE usage (using a script) and another manually
removes HERE declarations.

[rousskov]

This change has not been tested beyond basic build/startup tests.

I am not sure it should be committed (now) but I wanted to share it so that we can finally make this decision.

The script to remove HEREs from debugs() calls is available elsewhere.

[yadij]

most of these issues should be fixed either by making the script skip the relevant file entirely, or by actively adjusting (eg for the 'broken whitespace' notes).

[yadij]

Another example of why HERE cannot be scripted this way.

The DBG_CRITICAL is missing an ERROR or FATAL log prefix, and the hard-coded text "DiskThreadsDiskFile::close: " needs replacing with MYNAME.

[rousskov]

Same answer: This PR automatically removes HERE. Nothing else should be required.

[yadij]

"Who are you and what have you done with our Alex?"

Seriously though. When kinkie presented a script to do exactly these same HERE replacements, it was you vetoing its application with your insistence on a "perfection or nothing" policy to acceptable changes. Now you are accepting blatantly malformed code lines just because a script left it that way. So please, explain what is behind this complete reversal in your arguments?

[rousskov]

First of all, the script does not create blatantly malformed lines. It does not fix all the problems, but it does not create problems (bugs notwithstanding).

FWIW, I do not recall @kinkie presenting an equivalent solution. That does not mean it did not happen, of course! Please feel free to either encourage him to submit a competing PR or just use my old arguments against me in this PR if you think I was right back then. If I was right back then, then your (i.e., my old) arguments should win. If I was wrong back then, they should be defeated. We are here to select the best solution (among the available ones) and reject bad ones. It should not matter who says something. Our decisions should be based on the merits.

Finally, please note that I started this PR with an explicit disclaimer that it is not clear whether doing these changes is a good idea. If we decide that it is better to keep the status quo, I am content with that. I offered a reasonable alternative so that we can make an informed choice and close this topic one way or the other.

P.S. Please, if at all possible, when you attribute some old actions or decisions to people, please include references. It is trivial to misinterpret something, and context matters.

[yadij]

Overall I think this approach is wrong. To do scripted changes we should be applying a patch that adds a working script to scripts/ folder and updates the source-maintenance.sh script to run the cleanup sub-script. Of course the script needs a review of its generated changes before PR approval.

Using the above method avoids forcing everyone submitting patches to apply yet another script just in case the audit and QA has issues. The requirement to run source-maintenance as a pre-PR check is still relevant to find small issues.

Note that this still precludes abruptly dropping the macro definition - we have a long transition period still ahead while old code gets ported upwards and its existence is there to ease that process. Finally reaching agreement or tipping-point on the bulk conversion is only day 2 of a multi-year process.

FYI: I am in the process of adjusting the source-maintenance to submit PR with changes rather than straight committing them, so we still get review of bulk changes at that stage. It is currently identifying about a dozen things the recent PR have done wrong and auditors did not spot manually. So dropping it entirely at this stage does not appear to be a good option.

[rousskov]

Overall I think this approach is wrong.

I do not know what you mean by "this approach", but I think that we only have two in-scope options on the table:

1. Automated one-time removal of all HEREs. This PR uses this option.
2. Manual case-by-case removal of HERE when the corresponding line is changed for other reasons. We have been doing exactly that for a few years.

I do not see a third viable option, but I would be happy to hear it.

To do scripted changes we should be applying a patch that adds a working script to scripts/ folder and updates the source-maintenance.sh script to run the cleanup sub-script.

That does not compute for me. After we remove HERE, there will be no more HERE to remove. The code will not compile if somebody sneaks in HERE!

We can certainly add code and/or tools to police and/or format debugs() statements, but that work is outside this PR scope (and needs preliminary discussions). Removal of HERE has already been discussed, including the automated removal option.

Using the above method avoids forcing everyone submitting patches to apply yet another script just in case the audit and QA has issues.

This PR does not force anybody to apply any new scripts to any patches either.

Note that this still precludes abruptly dropping the macro definition - we have a long transition period still ahead while old code gets ported upwards and its existence is there to ease that process.

I propose to drop HERE definition from master. Any PRs containing HERE that want to land in master after this PR is merged can be easily fixed IMO. I see no significant value in "easing the process" of sneaking in debugs() lines with HERE into master, and I certainly see harm from doing so -- we do not want to manually police PRs for HERE presence. Can you give a few specific examples where removing HERE definition from master would create serious problems?

I think we should also drop HERE from v4 but it is your call.

It is probably too late for v3.5.

FYI: I am in the process of adjusting the source-maintenance to submit PR with changes rather than straight committing them, so we still get review of bulk changes at that stage. It is currently identifying about a dozen things the recent PR have done wrong and auditors did not spot manually. So dropping it entirely at this stage does not appear to be a good option.

Sounds good. Long-term, missing source maintenance will become one of the pre-merge checks, very similar to the current git diff --check. Automated source maintenance will be applied to PRs automatically and/or upon author/reviewer request (to be determined/discussed). There will not be any "SourceFormat Enforcement" commits in master!

[rousskov]

To summarize the current status: I believe that HERE usage removal should be 100% automated. I am OK with including more adjustments that can be easily automated together with HERE removal. I have identified a few such additional adjustments in the by-line comments.

1. If you agree with this approach, then please respond to those by-line comments discussing additional adjustments. Once we settle on what should be adjusted further, I will change the script accordingly and update this PR.
2. If you insist that this PR should combine automated HERE usage removal with difficult-to-automate and/or manual polishing of debugs() statements, then I will not make any changes (but may appeal for other opinions).

@yadij, how do you want to proceed?

[rousskov]

@yadij, now that we are branching off v5, it would be a good time to revisit removal of HERE tokens. If you are ready to accept the natural limitations of the HERE removal script, I can update it to work for the current code to get rid of HERE in both v5 and master.

[yadij]

I have been holding off on re-evaluating this until the PR #531 is merged. After that this huge changeset reduces down to a few lines adding replacement rules to the code filter.

[rousskov]

I have been holding off on re-evaluating this until the PR #531 is merged.

I see. Unfortunately, I doubt we should remove HERE from source-maintenance.sh (or the scripts it loads/calls). I did not discuss that in PR 531 because I felt that you wanted PR 531 changes to be general (i.e. not specific to HERE removal). I am still trying to find the time to post a specific suggestion on how to address wide-scope changes like HERE and NULL removal (and a lot more!). Maybe this week.

[rousskov]

#653 is competing with this old PR. I believe that, out of the two options, this PR's approach is the best way forward. I summarized those thoughts in #653 (review). If I am right, then I would be happy to sync this old PR (and the corresponding script) with the current code and, in a separate PR, remove any exceptional HEREs that require manual attention.

[rousskov]

The same scripting approach is now used in #881. No need to keep this stale PR open anymore.

[yadij]

FYI, I left this open because the manual changes here are not part of #881.

[rousskov]

FYI, I left this open because the manual changes here are not part of #881.

I hope you left this open because one should not close PRs opened by other active core developers.

Manual removal of the HERE definition (i.e. cd0b395) is a trivial matter not worth keeping this out-of-date PR open for.