refactor: misc style changes

[div72]

No description provided.

[barton2526]

Can this be done as well?
bitcoin/bitcoin#10645

[jamescowens]

@cyrossignol You ok with merging a style change refactor this broad? To me, now is as good a time as any. It has no conficts at the moment.

[jamescowens]

Good job on this. These are always a PITA. In general for the class parameters after the colon, on the next line and using the standard indent is fine. I have made comments where braces need to go after the ifs or if ... elses and also for's. Note that the code is chock full of the indent single line ifs and if ... elses without braces, so I only commented on ones that were near the changes you already made. I don't think it is necessary to go through the code exhaustively to look for those. Just fix what is near the changes you already made.

Some of this code is merged from upstream (Bitcoin) as you know, and so some of the changes here will likely be lost for that unless they have been fixed in the source.

[cyrossignol]

@jamescowens As you might see, these are scripted changes using automation from upstream. I think I agree with all of your suggestions, but many of those will need to be adjusted by hand, and that seems like it may take much longer than @div72's original patch. In the spirit of this PR, it may be worth looking into whether scripted-diffs exist for the suggestions rather than switching to manual mode. Large-scale cosmetic trivia doesn't really seem worth the time without some kind of automation.

[jamescowens]

I agree with you @cyrossignol. That is the trouble with scripted changes. They aren't really intelligent. Most of the changes are actually fine, except the after the colon alignment, which I think makes things worse there. We don't have to straighten up the missing braces for the ifs and elses, although it would be nice to have a script to do that...

[jamescowens]

I still think this is a worthwhile PR to merge... just to get rid of those antiquated and unsightly (*foo).fum's.

[div72]

Could we write the current preferred style of code somewhere(like docs/coding.txt) and enforce it? Then I can update the .clang-format file which should hopefully format better.

[jamescowens]

We already have. We follow the Bitcoin standards except that if (condition) { statement; } else { statement; } is acceptable as an alternative to if (condition) { statement; } else { statement; }

…

Sent from my iPhone

On Jun 16, 2021, at 8:38 AM, div72 ***@***.***> wrote:  Could we write the current preferred style of code somewhere(like docs/coding.txt) and enforce it? Then I can update the .clang-format file which should hopefully format better. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

[jamescowens]

Also, much of the older Bitcoin code does NOT meet their own (and our) styling standards. In general we have applied a pragmatic approach to cleanup of the older code. Unless the situation is gross, we have left older Bitcoin code undisturbed unless we are already modifying an area for other purposes. This limits PR conflicts.

As you will see in the newer code, we have been trying to adhere to the style guidelines much better.

[div72]

There should be one and only clear way to write something in my opinion. The { should either be on the same line or the next line in all newly written code.

Also, yes there is a style noted in docs/coding.txt but it is outdated and incomplete when compared with thr current gridcoin-specific code. New variables are named snake_case without type prefix where it notes PascalCase with type prefix, members variables are prefixed with m_, but it isn't noted etc.

You shouldn't have to say "This should be written like this...", the lint job should be configured and automatically fail if someone introduces a different style in a PR in my opinion at least.

[jamescowens]

I thought we had pulled over the Bitcoin code style document to replace the older one. Seems we didn't. That is what we are supposed to be following at this point. If you want to do a PR to bring that over, that would be wonderful.

I agree with your sentiment, but in many cases it is not realistic. Many PR's are not independent: instead they modify or extend code that has already been written. In many cases one has to use variables that have already been created, etc. These are often using legacy styles. In particular the legacy Qt side of the wallet is significantly different than the coding style of the core wallet. The core wallet has several styles depending on the era of the code. One has to make a choice, do you start refactoring to cause the older code that you are revising or extending to conform to the new style guide? The answer is sometimes yes, if the extension or modification you are trying to do (not including simply style revision) amounts to a complete overhaul of an area, but most of the time no, as excessive revision of older code for style issues only results in larger PR's involving more source files and is more likely to lead to conflicts with existing work that is underway.

Some of the reasons for the evolution in the C++ styling between the code image that was used when this wallet was amalgamated from Bitcoin/Peercoin/Blackoin and today is the changes in the tools used for coding. Hungarian notation or similar is not as relevant anymore because of the better, context aware development environments with significant parsing/checking capabilities, which render the need to indicate the type of variable in the variable name itself superfluous. The m_ prefix is a good idea to clearly distinguish member variables of a class, especially in a codebaselike ours which has quite a bit of global scope code that is not classed. Missing braces in situations where the combination of a missing brace and misleading indentation can lead to difficult to find bugs is more important than exact where the opening brace is placed IMHO.

I didn't mean to irritate you with this PR. I certainly think it improves things in many areas, and certainly doesn't do any harm. I am ok with merging it as is quite frankly. @cyrossignol?

[div72]

I apologize if I am a bit grumpy, been a bit busy and tired lately.

I kind of understand what you mean and which is why I think human assisted diff formatting is good.

Also sorry for the merge commit had to do it on mobile.

[jamescowens]

I think this is an improvement and is fine to merge now. @cyrossignol please do another pass on this too.

[cyrossignol]

Thanks @div72... quite a PR here...

As an aside, these are the rough guidelines for managing legacy code style that seem standard in most teams that I've worked on:

• Code in new modules should always follow the style guide
• When modifying legacy code, be consistent with the style of the surrounding scope
  • Typically, a function boundary, but it could be a small file/class or a large nested block
  • Prefer to avoid mixing cosmetic changes with domain changes (batch-commit formatting as a whole separately)
  • New scopes can follow the style guide to ease expected transitions later
• Do not modify the style of upstream code
• Rely on tools for style enforcement rather than manual review
  • Use linters to flag style violations
  • Use formatters to apply style before submission
  • "Policy, not personal"

Of course, we need an up-to-date style guide for any of this to really matter. I think we do a decent job following these points so far (I surely haven't done so absolutely myself). I mention these because formalizing our position should save us time—I'd rather discuss and review style changes as little as possible. No convention or tool set is perfect, though. The goal is to minimize the distraction of code style from the review process—both for new reviews (PRs) and historical (git log/blame, change tracking, etc.)—so I propose these guidelines as a starting point.

[jamescowens]

I agree @cyrossignol. Merging.