developer-notes: allow lowerCamelCase

[ryanofsky]

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.

[ryanofsky]

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

[kostyantyn]

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

[ryanofsky]

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]

utACK 6e3f8c2.

[sipa]

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]

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

Conflicts

No conflicts as of last run.

[promag]

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

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

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

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]

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

/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

[laanwj]

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

[maflcko]

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

[laanwj]

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

[Sjors]

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.

[maflcko]

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

[jamesob]

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

[Sjors]

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

[jamesob]

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

[ryanofsky]

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.