developer-notes: allow lowerCamelCase

ryanofsky commented at 9:18 pm on November 1, 2018: member

This PR changes developer notes to suggest naming instance methods with lowerCamelCase(), and other functions with UpperCamelCase().

Lower camel case names were allowed by the developer guide before #10917, and are used in a good amount of existing code.

Using lower camel case to distinguish instance method calls from other types of calls can aid with code readability and review. Seeing a lowerCase() call would tell you that an implicit this pointer is being passed, and could be a signal during code review that a call has access to more information than needs, or may not be doing exactly what you think it does.

Using a distinct style for instance method names is also analagous with our existing convention for using m_ prefixes in instance member variable names.

developer-notes: allow lowerCamelCase

Include rationalization, and rationalizations for existing m_ / g_ conventions.

6e3f8c201e

ryanofsky commented at 9:23 pm on November 1, 2018: member

Since #10917, developer notes have suggested using UpperCamelCase() for all function names, but a lot of existing code is using lowerCamelCase() naming for member functions.

This is prevalent in src/qt/ and src/interfaces/, but also common in some other places like policy/fees.h, txmempool.h, and scheduler.h: https://gist.github.com/ryanofsky/8718089b2927912f77db0d7cb1728350

fanquake added the label Docs on Nov 1, 2018

in doc/developer-notes.md:74 in 6e3f8c201e outdated

70@@ -71,11 +71,15 @@ required when doing so would need changes to significant pieces of existing
71 code.
72   - Variable (including function arguments) and namespace names are all lowercase, and may use `_` to
73     separate words (snake_case).
74-    - Class member variables have a `m_` prefix.
75-    - Global variables have a `g_` prefix.
76+    - Class member variables have a `m_` prefix, so it's obvious when variable

kostyantyn commented at 10:52 pm on November 1, 2018:

WDYS to drop m_ prefix if it’s the public member variable of a class/struct?

ryanofsky commented at 11:03 pm on November 1, 2018:

Maybe something to bring up in IRC or a separate issue. Personally I wouldn’t mind it, as long as I could tell by sight when a member variable was being accessed (with inst.var or this->var, or m_var or whatever), and not confuse it for a local or global variable.

promag commented at 11:12 pm on November 1, 2018: member

utACK 6e3f8c2.

sipa commented at 11:22 pm on November 1, 2018: member

I don’t personally object to a different style for instance methods and functions, but this makes the gap between the existing code and the suggested style much larger, as most of the codebase is using UpperCamelCase for instance methods (except for the qt and interfaces directories).

So I’m not sure this is desirable - the end goal should be getting the codebase to converge on a single style, and while that’s inevitably a slow process (as we don’t permit purely style changing patches), this moves that goal even further away.

My preference would be either restricting lowerCamelCase to specific directories (so that it roughly corresponds with existing usage), or not do it at all and suggest that new code even in places with lowerCamelCase usage use UpperCamelCase in the future.

DrahtBot commented at 11:40 pm on November 1, 2018: member

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

Conflicts

No conflicts as of last run.

promag commented at 11:42 pm on November 1, 2018: member

@sipa I don’t think the gap size matters, either way it will be a long way to converge. Also, this style is not in qt and interfaces directories only as you can see in the above gist.

I think @ryanofsky gives a good reason for this change.

sipa commented at 11:54 pm on November 1, 2018: member

@promag In my view, writing a particular guideline in the document implies that we’re as a project committing to (eventually) adopting it. Choosing a guideline that mismatches the existing code is giving ourself extra work in fixing it (and while that is ongoing, live with the inconsistencies in the codebase).

The question is whether the suggested style is enough of an improvement over the existing guidelines that it warrants all that work.

promag commented at 0:05 am on November 2, 2018: member

@sipa I agree with you except in

Choosing a guideline that mismatches the existing code is giving ourself extra work in fixing it.

there is already a lot of code to update to follow current guideline and it doesn’t matter, hence the no style changing patches.

If we were to start from scratch, this change LGTM.

ryanofsky commented at 0:20 am on November 2, 2018: member

Didn’t we already cross this bridge? Maybe I have a different perception, but it seems to me the switch from strLabel to m_label is bigger than a switch from UpdateLabel to updateLabel. @sipa I’d be interested to know if you agree with that, or think allowing inconsistencies in the first place was a mistake, or if you just think benefits are greater in one case than the other.

On commitments to guidelines, I tend to think it’s ok to have some guidelines that are just encouraged best practices and not enforced with a heavy hand, so maybe I can tweak this to weaken the suggestion a little.

On “specific directories”, I don’t really the love the idea of having different code styles for different directories. But a variation of that idea where we have a single, preferred style and a list of files/directories we say, “this code is mature and was originally written in a different style, so please don’t apply new guidelines here” maybe could be reasonable.

sipa commented at 0:30 am on November 2, 2018: member

@ryanofsky I consider the distance between the suggested style and the actual code as a metric for code consistency. I think code consistency is more important than any particular style choice itself, unless that style has objectively been shown to improve code quality (for example, actual high-impact vulnerabilities have existed that were attributable to if-with-indented-then-but-no-braces). In this specific case, all other things aside I think having methods look different from functions may be somewhat beneficial, but it doesn’t weigh up against introducing an extra way to go to make everything consistent.

As far as the existing snake_case / hUngarian discrepancy in the codebase goes, it was an intentional choice that I believe many people were on board with, as Hungarian style was the original, but even before the time snake_case became suggested, hardly anyone was following hUngarian. However, I think that everything else in the style document was written to match the existing code, rather than writing a style guide that would be optimal for a from-scratch project.

I would be okay with “these parts originated as separate projects which are quite mature now, so we’re continuing their style there”.

laanwj commented at 12:12 pm on November 5, 2018: member

/src/qt has always had a different style and mimicked Qt’s own style, I would not suggest using that as a guideline for changing the rest or even as a guideline for new code. mentioning this as a subdirectory-specific thing makes a lot of sense

developer-notes: Allow some leeway in style

This is in response to concern in https://github.com/bitcoin/bitcoin/pull/14635
comments about style recommendations causing unnecessary inconsistency and
churn in existing code.

4a3f13b07e

laanwj commented at 12:58 pm on November 12, 2018: member

Still vaguely NACK on this, I think the current naming style for methods is fine.

MarcoFalke commented at 3:54 pm on November 12, 2018: member

I am fine with this, but I won’t argue either way.

laanwj commented at 6:37 am on December 15, 2018: member

Going to close this, this is controversial and not making progress.

laanwj closed this on Dec 15, 2018

Sjors commented at 10:00 am on December 15, 2018: member

There’s some baby in this bath water in case anyone picks this up later. You could drop the sentence about lowerCamelCase and explain that src/qt (sometimes) follows QT convention.

ryanofsky referenced this in commit f1099a6822 on Apr 11, 2019

MarcoFalke commented at 7:36 pm on May 15, 2019: member

Looks like people are starting to use this-> (https://github.com/bitcoin/bitcoin/pull/15976/files#diff-24efdb00bfbe56b140fb006b562cc70bR2049)

jamesob commented at 7:42 pm on May 15, 2019: member

I’m happy with either lowerCamel or this->, but I do think that we should distinguish method calls from global calls somehow.

Sjors commented at 1:36 pm on May 16, 2019: member

@jamesob wrote:

I’m happy with either lowerCamel or this->, but I do think that we should distinguish method calls from global calls somehow.

Ditto. One argument for lowerCamel is that unlike this-> you can’t forget to use it. @laanwj wrote:

/src/qt has always had a different style and mimicked Qt’s own style, I would not suggest using that as a guideline for changing the rest or even as a guideline for new code.

Also agree with that.

ryanofsky referenced this in commit 5c084bb3b9 on Mar 9, 2021

DrahtBot locked this on Dec 16, 2021

fanquake unlocked this on Apr 12, 2022

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

@ryanofsky care to reopen? (based on discussion in #24008 (review))

ryanofsky commented at 11:28 pm on April 12, 2022: member

I’m happy to review someone else’s PR but I think this one is a little out of date and it’s not something I want to focus effort on. Also, this PR is very maximalist because when I opened it I was under the impression that #10917 just forgot about member functions when it replaced “CamelCase” with “UpperCamelCase”. (Nothing in the #10917 description or discussion says anything about establishing new naming rules. The only reasoning it cites at all is that the previous text was “ambiguous”.) So I didn’t realize anyone would have a problem with the recommendations here, and at this point some different, or more minimal approach that just allows more flexibility might make more sense.

DrahtBot locked this on Apr 12, 2023

developer-notes: allow lowerCamelCase #14635

Conflicts