developer-notes: allow lowerCamelCase for methods

jamesob commented at 4:48 pm on April 13, 2022: member

Replaces #14635

This naming style allows calls to easily convey whether or not they are to methods of the current class instance.

This style is (and continues to be) used extensively in src/node, src/interfaces, and other parts of the code.

jamesob force-pushed on Apr 13, 2022

fanquake added the label Docs on Apr 13, 2022

in doc/developer-notes.md:92 in 5e1a73436e outdated

87@@ -88,15 +88,22 @@ required when doing so would need changes to significant pieces of existing
88 code.
89   - Variable (including function arguments) and namespace names are all lowercase and may use `_` to
90     separate words (snake_case).
91-    - Class member variables have a `m_` prefix.
92-    - Global variables have a `g_` prefix.
93+    - Class member variables have a `m_` prefix, so it's obvious when variable
94+      references in the code implicitly access the current class instance.

ryanofsky commented at 6:01 pm on April 13, 2022:

MIght be better to be shorter “so it’s obvious when code is accessing implicit instance member variables.”

jonatack commented at 6:27 pm on April 13, 2022:

Even pithier (here and below): s/, so it's obvious/ to indicate/

ajtowns commented at 9:33 pm on April 13, 2022:

I don’t agree that that’s the reason for using m_ or g_; in my opinion the main benefit is that it avoids shadowing, so that parameters/local variables don’t hide access to member variables or globals. In C++ any non-static member function implicitly has access to the current class instance, and you hopefully already know what function you’re looking at.

in doc/developer-notes.md:102 in 5e1a73436e outdated

101     which recommend using `snake_case`.  Please use what seems appropriate.
102-  - Class names, function names, and method names are UpperCamelCase
103-    (PascalCase). Do not prefix class names with `C`.
104+  - Type/class names, standalone function names, and static method names are
105+    UpperCamelCase. Instance method names are encouraged to be lowerCamelCase, so 
106+    it's obvious when function calls in the code implicitly access the current class

ryanofsky commented at 6:10 pm on April 13, 2022:

Might be better to be shorter “so it’s obvious when code is calling implicit instance member functions”

ryanofsky approved

ryanofsky commented at 6:19 pm on April 13, 2022: contributor

ACK to any version of this (but FYI trailing whitespace lint https://cirrus-ci.com/task/5196882757550080 in 5e1a73436e5f19b0c131b181affa8e01e36d9ee4)

in doc/developer-notes.md:105 in 5e1a73436e outdated

104+  - Type/class names, standalone function names, and static method names are
105+    UpperCamelCase. Instance method names are encouraged to be lowerCamelCase, so 
106+    it's obvious when function calls in the code implicitly access the current class
107+    instance. Class names should not be prefixed with `C`.
108+    - If an existing method name is named with UpperCamelCase, `this->` may be used 
109+      as a prefix when it to readily distinguish it from a function, e.g.

jonatack commented at 6:29 pm on April 13, 2022:

“when it to”?

in doc/developer-notes.md:101 in 5e1a73436e outdated

100     Guidelines](https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Renum-caps),
101     which recommend using `snake_case`.  Please use what seems appropriate.
102-  - Class names, function names, and method names are UpperCamelCase
103-    (PascalCase). Do not prefix class names with `C`.
104+  - Type/class names, standalone function names, and static method names are
105+    UpperCamelCase. Instance method names are encouraged to be lowerCamelCase, so

jonatack commented at 6:32 pm on April 13, 2022:

I’m not sure about this other than in classes where it is already the naming style. Would be good to avoid naming changes of existing code for this.

laanwj commented at 6:39 pm on April 13, 2022:

For what it’s worth, I agree.

ryanofsky commented at 6:41 pm on April 13, 2022:

I’m not sure about this other than in classes where it is already the naming style. Would be good to avoid naming changes of existing code for this.

Agree but I think this is why it says “are encouraged to be” instead of just “are”. Personally, I would be happy with “can”. People already use judgment about how much to clean things up and how much to leave things alone when modifying code. This could just be a nudge in a useful direction.

jonatack commented at 6:56 pm on April 13, 2022:

Looking for a compromise but not advocating, maybe “can” as an exception or where already the style but no renamings for it. If people agree, that is.

laanwj commented at 6:35 pm on April 13, 2022: member

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.

jonatack commented at 6:44 pm on April 13, 2022: contributor

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

ryanofsky commented at 6:45 pm on April 13, 2022: contributor

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?

jamesob commented at 6:54 pm on April 13, 2022: member

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.

sipa commented at 7:02 pm on April 13, 2022: member

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

laanwj commented at 7:03 pm on April 13, 2022: member

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)

jamesob commented at 7:12 pm on April 13, 2022: member

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.

sipa commented at 7:25 pm on April 13, 2022: member

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

ryanofsky commented at 8:53 pm on April 13, 2022: contributor

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.

ajtowns commented at 9:28 pm on April 13, 2022: contributor

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#pullrequestreview-40884129

ajtowns commented at 9:50 pm on April 13, 2022: contributor

If we want to maintain a different style in the qt code, that seems fine to me and worth documenting.

Changing the style for the remaining code to something that’s been rejected twice seems nuts, and if it’s been sneaking in as part of assumeutxo work, that seems like kind of a worrying indication that there’s been far too little review of that code, rather than a reason to encourage everyone else to switch to its standard.

I don’t think this has a benefit that’s worth adding another thing on the “let’s convert all the code to style Y instead of X” todo list. (Same is true for qt/* with X and Y swapped) I do think having a convention that everyone tries to use for new code is better than each individual doing what they like best for their code.

So -1 from me.

developer-notes: allow lowerCamelCase for methods

This naming style allows calls to easily convey whether or not they are
to methods of the current class instance.

This style is (and continues to be) used extensively in src/node,
src/interfaces, and other parts of the code.

Feedback from jonatack.

Co-authored-by: Russell Yanofsky <russ@yanofsky.org>

755c29d86d

jamesob force-pushed on Apr 13, 2022

jamesob commented at 10:12 pm on April 13, 2022: member

Updated to include feedback from @jonatack @ryanofsky.

MarcoFalke commented at 7:00 am on April 14, 2022: member

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 (https://github.com/bitcoin/bitcoin/blob/8e3c266a4f02093d57d563f32ba73d3ab4b5f208/src/.clang-tidy#L2) and not a style guide that is discouraged to be enforced.

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?

jonatack commented at 7:57 am on April 14, 2022: contributor

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.

ajtowns commented at 6:08 am on April 22, 2022: contributor

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.

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)
Have different .clang-tidy configs for each style we want to try to be consistent with
Have *IgnoredRegexp for any exceptions
Document the current failures, and have a python script that catches new failures
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:

 0CheckOptions:
 1  - { key: readability-identifier-naming.NamespaceCase,       value: lower_case }
 2
 3  - { key: readability-identifier-naming.NamespaceIgnoredRegexp,       value: '^(Consensus|BCLog|NetMsgType|RPCServer)$' }
 4
 5  - { key: readability-identifier-naming.ClassCase,           value: CamelCase }
 6  - { key: readability-identifier-naming.StructCase,          value: CamelCase }
 7  - { key: readability-identifier-naming.MethodCase,          value: CamelCase }
 8  - { key: readability-identifier-naming.EnumCase,            value: CamelCase }
 9
10  - { key: readability-identifier-naming.ClassIgnoredRegexp,  value: '^([a-z0-9]+_error|reverse_lock|base_blob|uint256|uint160)$' }
11  - { key: readability-identifier-naming.StructIgnoredRegexp,  value: '^(deserialize_type|bilingual_str|[a-z]+_iter|is_Span.*)$' }
12
13  - { 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]*_)$' }
14
15  - { key: readability-identifier-naming.ClassMemberCase,     value: lower_case }
16  - { key: readability-identifier-naming.ConstantMemberCase,    value: lower_case }
17  - { key: readability-identifier-naming.PrivateMemberCase,     value: lower_case }
18  - { key: readability-identifier-naming.PrivateMemberPrefix,   value: m_ }
19  - { key: readability-identifier-naming.ProtectedMemberCase,   value: lower_case }
20  - { key: readability-identifier-naming.ProtectedMemberPrefix, value: m_ }
21  - { key: readability-identifier-naming.PublicMemberCase,      value: lower_case }
22  - { key: readability-identifier-naming.PublicMemberPrefix,    value: m_ }
23
24#  - { key: readability-identifier-naming.GlobalConstantCase,  value: lower_case }
25  - { key: readability-identifier-naming.StaticConstantCase,  value: lower_case }
26
27  - { key: readability-identifier-naming.ConstexprVariableCase,  value: UPPER_CASE }
28  - { key: readability-identifier-naming.ClassConstantCase,   value: UPPER_CASE }
29  - { key: readability-identifier-naming.EnumConstantCase,    value: UPPER_CASE }
30
31  - { 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.

MarcoFalke commented at 6:19 am on April 22, 2022: member

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.

ajtowns commented at 6:37 am on April 22, 2022: contributor

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.

DrahtBot commented at 2:28 am on May 11, 2022: contributor

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.

DrahtBot added the label Needs rebase on May 16, 2022

DrahtBot commented at 8:50 am on May 16, 2022: contributor

🐙 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”.

jamesob closed this on Jul 18, 2022

bitcoin locked this on Jul 18, 2023

developer-notes: allow lowerCamelCase for methods #24846

Conflicts