I still agree with what I wrote in #14635 (comment), I really don't think we need another method naming style. There are a few places where the style is already used (such as in the GUI), we should keep using it there, but in core we use UpperCamelCase quite consistently and it would be worse to start mixing it.

As I mentioned on IRC yesterday, the value to me of documenting this is to provide context for reviewers, i.e. when coming across code with this naming style over the past years but not having the context, I thought it was just the legacy style (or the Qt style).

(I don't have a strong opinion about the naming style other than it seems best to stick with the same one for new and changed code to avoid renaming changes and discussions about it. This probably outweighs any advantage of one or the other, though again, no strong opinion; I'd just like it to be sort of clear if possible and minimize renamings.)

would be worse to start mixing it

This is either begging the question, or I'm missing something. Worse in what way? Aesthetically? Or is it worse to draw attention to the fact that a call is using implicit *this?

There are a few places where the style is already used (such as in the GUI)

@laanwj I'm confused because there are many more places in the code aside from the GUI where this convention is actively used, e.g. pretty much anything touched during the multiprocess work and some recent assumeutxo changes.

@laanwj I'm confused because there are many more places in the code aside from the GUI where this convention is actively used, e.g. pretty much anything touched during the multiprocess work and some recent assumeutxo changes.

Given the fact that apparently large amounts of code sneaked in and were accepted and merged while blatantly violating the coding style as written in this document, I think it's fair to conclude that the project doesn't enforce a coding style, and this document is obsolete.

This is either begging the question, or I'm missing something. Worse in what way? Aesthetically? Or is it worse to draw attention to the fact that a call is using implicit *this?

Worse in the sense that we've used a certain convention for a long time, and I don't see a reason to aggressively push for a new one. But I wont comment here anymore I don't feel like bikeshedding style. Do whatever you like.

(to be clear I don't think distinguishing static and non-static methods is a bad idea in itself, nor is lowerCamelCase in general, I just don't think it's useful enough of a change to start moving the entire codebase to it by recommending it for new code everywhere, to be worth introducing more inconsistency even temporarily)

Given the fact that apparently large amounts of code sneaked in and were accepted and merged while blatantly violating the coding style as written in this document, I think it's fair to conclude that the project doesn't enforce a coding style, and this document is obsolete.

Whoa, I think this is a little hyperbolic... This document is still very useful for (i) supporting review feedback ("change this style please") and (ii) for new contributors looking to learn what to conform to.

I promise I'm not trying to stir up bad feelings with this PR, but I just wanted to reconcile the fact that this document seems out of date with a lot of recently added code; I thought that this style change had been "de facto" accepted based on all the code added.

If the project maintainers really feel that this (I think useful?) convention should be avoided, I trust that @ryanofsky and I will stop adding code that violates the desired style policy.

(i) supporting review feedback ("change this style please")

Well, apparently the document style document is seen as more descriptive ("these is/are the coding styles people have been trying to use recently in this repository") than prescriptive - this PR itself being evidence of that.

I promise I'm not trying to stir up bad feelings with this PR, but I just wanted to reconcile the fact that this document seems out of date with a lot of recently added code; I thought that this style change had been "de facto" accepted based on all the code added.

I'm not blaming you (or anyone) for what happened. But to me, the damage is done. My hope was that by having a well-documented coding style years ago, we could start converging on a single style. If having such a document isn't enough to get people to follow it, then that's a sign we simply don't enforce coding styles. That's ok - it can stay as a description of the used style rather than defining the rules people should follow - and in that sense it's great that you're trying to reconcile them.

If the project maintainers really feel that this (I think useful?) convention should be avoided, I trust that @ryanofsky and I will stop adding code that violates the desired style policy.

I don't have any problems with lowerCamel or distinguishing member functions from non-member functions, but changing the policy is just a step backwards in terms of getting the style consistent, and consistency was more important to me than individual style decisions.

There are also reasons that make it inherently hard to follow a consistent style too - beyond anyone's control even. For example, the documented style doesn't match STL's, so when code gets added that implements STL interfaces, it's forced to violate whatever style we have written up. So perhaps my hope wasn't realistic in the first place.

Given the fact that apparently large amounts of code sneaked in and were accepted and merged while blatantly violating the coding style as written in this document

I'm probably missing history because I haven't been around as long and don't love IRC, but from my perspective, this isn't what happened.

The guide said "CamelCase" not "UpperCamelCase" until July 2017 when #10917 changed it. It's not clear to me if #10917 accidentally added a new rule, or if was writing down a rule that developers were already aware of, but there was no discussion about code style in #10917, and existing code used a mix of cases with gui code, mempool code, and fee estimation code in particular using lowerCamelCase at that point.

The main code added since then which uses the lower case style is src/interfaces/, because that code started with the GUI PR #10244 and followed GUI style, and also because capnproto enforces a similar convention and I wanted to avoid translating method names.

I generally like to follow the style of surrounding code. I care as much about consistency as anyone else, but to me it's a lot worse if there is inconsistent style within a class or module, than if entirely different classes or libraries are using different styles.

we've used a certain convention for a long time, and I don't see a reason to aggressively push for a new one.

Agree with this. I don't want to push for a new style, just allow the old one. I also think it is good to add some rationale to the document for the various conventions, and to have some rationale to begin with.

The guide said "CamelCase" not "UpperCamelCase" until July 2017 when #10917 changed it. It's not clear to me if #10917 accidentally added a new rule, or if was writing down a rule that developers were already aware of,

This was brought up in the original PR that introduced "CamelCase" -- #10461 (review)

Updated to include feedback from @jonatack @ryanofsky.

Reminds me a bit about the last "change" to the document: #18568 (comment)

Some loose points, starting with a fun fact: Some methods in the code base are named snake_case, so should this be mentioned here as well?

Maybe instead of adding more rules and rationales, whose wording people will start to bike-shed itself, we should be removing them? Apparently the rules here didn't avoid bike-shedding among developers that are familiar with this project. Also, I doubt that any new contributor reads this file (and understands it well enough to keep in mind) before starting to contribute. The docs are also a bit contradictory when they say pure-style review-comments are not welcome and then there is a 65kB document prescribing describing the style. Overall I think any style guideline that is purely stylistic and doesn't help to fight bugs is a candidate for removal. And those that evidently fight bugs (like arg comments) are ideally enforced by tools (

The lowerCamelCase interface style is already described in this document (in the section "Internal interface guidelines"), but reading the previous comments on this discussion no one seems to be aware of this. Another data point that even regular contributors have difficulties keeping the ideas mentioned in the full length of this document in mind?

The lowerCamelCase interface style is already described in this document (in the section "Internal interface guidelines"), but reading the previous comments on this discussion no one seems to be aware of this. Another data point that even regular contributors have difficulties keeping the ideas mentioned in the full length of this document in mind?

It might be useful to mention and link to that section next to the PascalCase guideline, as it is an exception to it. I had noted that the section was delimited to src/interfaces and my comments here above refer to the rest of the codebase.

My hope was that by having a well-documented coding style years ago, we could start converging on a single style. If having such a document isn't enough to get people to follow it, then that's a sign we simply don't enforce coding styles.

We could use clang-tidy to start making CI error out when the style gets made more inconsistent. It's a bit challenging given we've already got a huge inconsistent mess of styles, but I think the following approach could be made to work.

1. Pick which modules follow "our" coding style, and which ones follow some other style (eg something more like the STL's style with snake_case, or qt/capnproto's lowerCamelCase style, or are imported from upstream like tinyformat or secp256k1 or leveldb)
2. Have different .clang-tidy configs for each style we want to try to be consistent with
3. Have *IgnoredRegexp for any exceptions
4. Document the current failures, and have a python script that catches new failures
5. Maybe only run the checks over the .h files initially so we at least trend towards our exported interfaces matching our style guide... (That results in clang-tidy complaining about various things being defined but not used, but that at least seems easy enough to strip out)

I think a .clang-tidy config something like:

CheckOptions:
  - { key: readability-identifier-naming.NamespaceCase,       value: lower_case }

  - { key: readability-identifier-naming.NamespaceIgnoredRegexp,       value: '^(Consensus|BCLog|NetMsgType|RPCServer)$' }

  - { key: readability-identifier-naming.ClassCase,           value: CamelCase }
  - { key: readability-identifier-naming.StructCase,          value: CamelCase }
  - { key: readability-identifier-naming.MethodCase,          value: CamelCase }
  - { key: readability-identifier-naming.EnumCase,            value: CamelCase }

  - { key: readability-identifier-naming.ClassIgnoredRegexp,  value: '^([a-z0-9]+_error|reverse_lock|base_blob|uint256|uint160)$' }
  - { key: readability-identifier-naming.StructIgnoredRegexp,  value: '^(deserialize_type|bilingual_str|[a-z]+_iter|is_Span.*)$' }

  - { key: readability-identifier-naming.MethodIgnoredRegexp,         value: '^(data|begin|end|write|read|seek|ignore|empty|size|reserve|eof|release|fclose|resize|rdbuf|in_avail|swap|[A-Z][a-zA-Z]*_)$' }

  - { key: readability-identifier-naming.ClassMemberCase,     value: lower_case }
  - { key: readability-identifier-naming.ConstantMemberCase,    value: lower_case }
  - { key: readability-identifier-naming.PrivateMemberCase,     value: lower_case }
  - { key: readability-identifier-naming.PrivateMemberPrefix,   value: m_ }
  - { key: readability-identifier-naming.ProtectedMemberCase,   value: lower_case }
  - { key: readability-identifier-naming.ProtectedMemberPrefix, value: m_ }
  - { key: readability-identifier-naming.PublicMemberCase,      value: lower_case }
  - { key: readability-identifier-naming.PublicMemberPrefix,    value: m_ }

#  - { key: readability-identifier-naming.GlobalConstantCase,  value: lower_case }
  - { key: readability-identifier-naming.StaticConstantCase,  value: lower_case }

  - { key: readability-identifier-naming.ConstexprVariableCase,  value: UPPER_CASE }
  - { key: readability-identifier-naming.ClassConstantCase,   value: UPPER_CASE }
  - { key: readability-identifier-naming.EnumConstantCase,    value: UPPER_CASE }

  - { key: readability-identifier-naming.ConstexprVariableIgnoredRegexp,  value: '^(deserialize|GetRandMicros|GetRandMillis)$' }

might be a plausible start, and I came up with the following list of .h files that seem like they don't currently target our style:

• script/bitcoinconsensus.h
• prevector.h
• indirectmap.h
• fs.h
• cuckoocache.h
• bloom.h
• bench/*
• minisketch/*
• crypto/*
• crc32c/*
• univalue/*
• secp256k1/*
• qt/*
• wallet/*
• leveldb/*
• interfaces/*
• ipc/*
• arith_uint256.h
• test/fuzz/*
• tinyformat.h
• reverse_iterator.h
• support/allocators/*

Putting that all together (and after changing a bunch of const FOO to constexpr FOO), I get about 900 remaining style errors... For what it's worth, switching MethodCase from CamelCase to camelBack bumps that number up to about 1900 style errors.

It seems a bit wasteful to maintain a linter that needs thousands of exceptions for no practical benefit. I'd doubt that the case of the fist character of the name of a function is the reason that end users will see less or more bugs or less or more features. Maybe if you have two function names in the same class or scope that only differ in case, but then a linter to detect those might be better suited.

It seems a bit wasteful to maintain a linter that needs thousands of exceptions for no practical benefit.

The practical benefit is that you don't need to manually review for new style errors that can be automatically caught. If we don't think working towards and then maintaining a consistent style is a practical benefit (personally, I do), then we should drop the style recommendations from the developer notes.

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #25092 (doc: various developer notes updates by jonatack)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

🐙 This pull request conflicts with the target branch and needs rebase.

_{Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft".}