doc: Clarify developer notes about constant naming #18568

ryanofsky · 2020-04-08T19:11:45Z

I'm pretty sure developer notes were intended to say constants should be upper case and variables should be lower case, but right now they are ambiguous about whether to write:

extern const int SYMBOL;

or:

extern const int g_symbol;

First convention above is better than the second convention because it tells you without having to look anything up that the value of SYMBOL won't change at runtime. Also I haven't seen other c++ projects using the second convention.

I'm pretty sure developer notes were intended to say constants should be upper case and variables should be lower case, but right now they are ambiguous about whether to write: ```c++ // foo.h extern const int SYMBOL; // foo.cpp const int SYMBOL = 1; ``` or: ```c++ // foo.h extern const int g_symbol; // foo.cpp const int g_symbol = 1; ``` First convention above is better than the second convention because it tells you without having to look anything up that the value of `SYMBOL` will never change at runtime. Also I've never seen any c++ project anywhere using the second convention

maflcko · 2020-04-08T19:55:21Z

Why is const int ... = 1; not a compile time constant?

ryanofsky · 2020-04-08T20:03:33Z

Why is const int ... = 1; not a compile time constant?

Can you give a more specific example? In the example from the PR description, the compiler does not know what the value of SYMBOL is when you include foo.h and use SYMBOL in compiled code, so it is misleading or at least a stretch to call SYMBOL a "compile time constant". In this example, SYMBOL is just const in the sense that it can't be modified after it is initialized and its value does not change at runtime.

jonatack

Concept ACK

doc/developer-notes.md

maflcko

In the example from the PR description, the compiler does not know what the value of SYMBOL is when you include foo.h and use SYMBOL in compiled code, so it is misleading or at least a stretch to call SYMBOL a "compile time constant"

While to compiler doesn't know its value, it does know that it is a constant, right? Maybe I've been using "compile time constant" interchangeably with "link time constant".

doc/developer-notes.md

ryanofsky

Updated 05f9770 -> 58ff41e (pr/capconst.1 -> pr/capconst.2, compare) to not imply local constants need to be capitalized.

I'm assuming that in some cases it makes sense to capitalize local constants, and in other cases it doesn't, and it isn't important to have a rule about it.

doc/developer-notes.md

maflcko · 2020-04-08T22:27:34Z

doc/developer-notes.md

@@ -83,7 +83,7 @@ code.
    separate words (snake_case).
    - Class member variables have a `m_` prefix.
    - Global variables have a `g_` prefix.
-  - Compile-time constant names are all uppercase, and use `_` to separate words.
+  - Global constant names are all uppercase, and use `_` to separate words.


Suggested change

- Global constant names are all uppercase, and use `_` to separate words.

- Global or compile-time constant names are all uppercase, and use `_` to separate words.

Since there could be non-global-scope compile-time constants, which should be UPPER_CASE?

I wonder the same. Class-scope constants should be the same, right? Even function-level constants would be better off in caps, _ separated imo. Do we have any notable exceptions here?

Edit: I think the exception is function arguments and things that aren't really constants but marked const to enforce that they shouldn't be changed. So the "compile-time" makes a lot of sense.

I second the extra specification of compile-time constants and the exclusion of run-time constants

I second the extra specification of compile-time constants and the exclusion of run-time constants

This PR is trying to drop the phrase "compile-time" because it doesn't have an obvious and clear meaning (see discussion in opening comments above), and because being "compile-time" seems less relevant to how a variable should be used or named compared to other attributes like mutability, scope, and lifetime. Also, I don't think in this document it is worth trying to be very technical or make detailed rules about how everything needs to be named. For local variables, it seems fine to just let developers have freedom to emphasize constness with caps where it seems better. Global variables are different because global variables are declared and accessed in different places, often far away, and if you are using one you almost always need to be aware if it has a constant value or if the value can change. Whether the value is initially evaluated at preprocessing time, compile time, link time, or perhaps early init can be interesting to know, but probably less relevant than knowing whether the value changes. A capitalized name is a good way to indicate that the value will not change.

Re: class constants, if it would help I could replace "global constants" with "global constants and other constants with static duration including namespaced and class-scope constants" here. But I don't think developer guidelines need to be so procedural or legalistic. I wouldn't expect someone just reading the updated text to be confused by it, and I wouldn't expect someone familiar with C++ to think of ::MY_CONSTANT, my_namespace::MY_CONSTANT, and MyClass::MY_CONSTANT much differently by default. Also, it seems fine to me if someone wants to use normal variable names for non-global constants. I think the benefit of using ALL_CAPS in a symbol name is to communicate the fact that the symbol's value will not change. If a developer chooses to not indicate or emphasize constness in some symbol name, it doesn't seem like a problem.

Maybe just remove "global" or is there a reason to imply function-scope constants should be lower snake case?

Maybe just remove "global" or is there a reason to imply function-scope constants should be lower snake case?

Removing "global" is really my preference, and "global" wasn't part of the original push which just dropped the "compile-time" phrase: pr/capconst.1. I added "global" later in pr/capconst.2 to try to address a concern about the rule sounding too broad (actually from you).

I think I'll switch back to the original push for now since it's pretty clear "global" doesn't help in the way I thought it might. Clarifying things is hard I guess. My goal is really just to drop the phrase "compile-time."

jarolrod

Concept ACK, pending the re-inclusion of compile-time constants in the changed line.

jarolrod · 2021-03-01T05:01:31Z

doc/developer-notes.md

@@ -83,7 +83,7 @@ code.
    separate words (snake_case).
    - Class member variables have a `m_` prefix.
    - Global variables have a `g_` prefix.
-  - Compile-time constant names are all uppercase, and use `_` to separate words.
+  - Global constant names are all uppercase, and use `_` to separate words.


I second the extra specification of compile-time constants and the exclusion of run-time constants

DrahtBot · 2021-05-13T10:43:32Z

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

Conflicts

No conflicts as of last run.

ryanofsky

Reverted 58ff41e -> 05f9770 (pr/capconst.2 -> pr/capconst.1, compare)

ryanofsky · 2021-06-29T16:57:27Z

doc/developer-notes.md

@@ -83,7 +83,7 @@ code.
    separate words (snake_case).
    - Class member variables have a `m_` prefix.
    - Global variables have a `g_` prefix.
-  - Compile-time constant names are all uppercase, and use `_` to separate words.
+  - Global constant names are all uppercase, and use `_` to separate words.


Maybe just remove "global" or is there a reason to imply function-scope constants should be lower snake case?

Removing "global" is really my preference, and "global" wasn't part of the original push which just dropped the "compile-time" phrase: pr/capconst.1. I added "global" later in pr/capconst.2 to try to address a concern about the rule sounding too broad (actually from you).

I think I'll switch back to the original push for now since it's pretty clear "global" doesn't help in the way I thought it might. Clarifying things is hard I guess. My goal is really just to drop the phrase "compile-time."

maflcko · 2021-06-29T18:16:37Z

cr ACK 05f9770

Current wording seems ok and attempts to make it more clear will only raise more concerns. Also, there doesn't seem too much disagreement in practice about this. The rules hare are more descriptive than prescriptive, so the exact wording shouldn't matter too much when most are doing the right thing anyway.

jarolrod

ACK 05f9770 🥃

sipa · 2021-06-29T19:36:29Z

ACK

practicalswift · 2021-06-29T22:20:54Z

ACK 05f9770

05f9770 doc: Clarify developer notes about constant naming (Russell Yanofsky) Pull request description: I'm pretty sure developer notes were intended to say constants should be upper case and variables should be lower case, but right now they are ambiguous about whether to write: ```c++ extern const int SYMBOL; ``` or: ```c++ extern const int g_symbol; ``` First convention above is better than the second convention because it tells you without having to look anything up that the value of `SYMBOL` won't change at runtime. Also I haven't seen other c++ projects using the second convention. ACKs for top commit: MarcoFalke: cr ACK 05f9770 practicalswift: ACK 05f9770 jarolrod: ACK 05f9770 🥃 Tree-SHA512: 766d0e25d9db818d45df4ad6386987014f2053584cbced4b755ceef8bda6b7e2cfeb34eb8516423bd03b140faaf577614d5e3be2799f7eed0eb439187ab85323

fanquake · 2021-06-30T02:08:08Z

Don't know why this wasn't autoclosed, but this has been merged.

05f9770 doc: Clarify developer notes about constant naming (Russell Yanofsky) Pull request description: I'm pretty sure developer notes were intended to say constants should be upper case and variables should be lower case, but right now they are ambiguous about whether to write: ```c++ extern const int SYMBOL; ``` or: ```c++ extern const int g_symbol; ``` First convention above is better than the second convention because it tells you without having to look anything up that the value of `SYMBOL` won't change at runtime. Also I haven't seen other c++ projects using the second convention. ACKs for top commit: MarcoFalke: cr ACK 05f9770 practicalswift: ACK 05f9770 jarolrod: ACK 05f9770 🥃 Tree-SHA512: 766d0e25d9db818d45df4ad6386987014f2053584cbced4b755ceef8bda6b7e2cfeb34eb8516423bd03b140faaf577614d5e3be2799f7eed0eb439187ab85323

ryanofsky mentioned this pull request Apr 8, 2020

build: Drop bitcoin-tx and bitcoin-wallet dependencies on libevent #18504

Merged

jonatack reviewed Apr 8, 2020

View reviewed changes

doc/developer-notes.md Show resolved Hide resolved

DrahtBot added the Docs label Apr 8, 2020

maflcko reviewed Apr 8, 2020

View reviewed changes

doc/developer-notes.md Show resolved Hide resolved

ryanofsky force-pushed the pr/capconst branch from 05f9770 to 58ff41e Compare April 8, 2020 22:03

ryanofsky commented Apr 8, 2020

View reviewed changes

doc/developer-notes.md Show resolved Hide resolved

maflcko reviewed Apr 8, 2020

View reviewed changes

jarolrod reviewed Mar 1, 2021

View reviewed changes

DrahtBot mentioned this pull request May 13, 2021

doc: update enumerator naming in developer notes #21930

Merged

ryanofsky force-pushed the pr/capconst branch from 58ff41e to 05f9770 Compare June 29, 2021 16:58

ryanofsky commented Jun 29, 2021

View reviewed changes

jarolrod reviewed Jun 29, 2021

View reviewed changes

fanquake closed this Jun 30, 2021

maflcko mentioned this pull request Nov 5, 2021

test: Parse command line arguments from unit and fuzz tests, make addrman consistency check ratio easier to change #23373

Merged

maflcko mentioned this pull request Apr 14, 2022

developer-notes: allow lowerCamelCase for methods #24846

Closed

gwillen pushed a commit to ElementsProject/elements that referenced this pull request Jun 1, 2022

Merge bfd910c into merged_master (Bitcoin PR bitcoin/bitcoin#18568)

35df560

bitcoin locked as resolved and limited conversation to collaborators Aug 18, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

doc: Clarify developer notes about constant naming #18568

doc: Clarify developer notes about constant naming #18568

ryanofsky commented Apr 8, 2020 •

edited

Loading

maflcko commented Apr 8, 2020 •

edited by ryanofsky

Loading

ryanofsky commented Apr 8, 2020 •

edited

Loading

jonatack left a comment

maflcko left a comment

ryanofsky left a comment

maflcko Apr 8, 2020

laanwj Apr 22, 2020 •

edited

Loading

jarolrod Mar 1, 2021

ryanofsky Jun 25, 2021

maflcko Jun 29, 2021

ryanofsky Jun 29, 2021

jarolrod left a comment

jarolrod Mar 1, 2021

DrahtBot commented May 13, 2021 •

edited

Loading

ryanofsky left a comment

ryanofsky Jun 29, 2021

maflcko commented Jun 29, 2021

jarolrod left a comment

sipa commented Jun 29, 2021

practicalswift commented Jun 29, 2021

fanquake commented Jun 30, 2021

	- Global constant names are all uppercase, and use `_` to separate words.
	- Global or compile-time constant names are all uppercase, and use `_` to separate words.

doc: Clarify developer notes about constant naming #18568

doc: Clarify developer notes about constant naming #18568

Conversation

ryanofsky commented Apr 8, 2020 • edited Loading

maflcko commented Apr 8, 2020 • edited by ryanofsky Loading

ryanofsky commented Apr 8, 2020 • edited Loading

jonatack left a comment

Choose a reason for hiding this comment

maflcko left a comment

Choose a reason for hiding this comment

ryanofsky left a comment

Choose a reason for hiding this comment

maflcko Apr 8, 2020

Choose a reason for hiding this comment

laanwj Apr 22, 2020 • edited Loading

Choose a reason for hiding this comment

jarolrod Mar 1, 2021

Choose a reason for hiding this comment

ryanofsky Jun 25, 2021

Choose a reason for hiding this comment

maflcko Jun 29, 2021

Choose a reason for hiding this comment

ryanofsky Jun 29, 2021

Choose a reason for hiding this comment

jarolrod left a comment

Choose a reason for hiding this comment

jarolrod Mar 1, 2021

Choose a reason for hiding this comment

DrahtBot commented May 13, 2021 • edited Loading

Conflicts

ryanofsky left a comment

Choose a reason for hiding this comment

ryanofsky Jun 29, 2021

Choose a reason for hiding this comment

maflcko commented Jun 29, 2021

jarolrod left a comment

Choose a reason for hiding this comment

sipa commented Jun 29, 2021

practicalswift commented Jun 29, 2021

fanquake commented Jun 30, 2021

ryanofsky commented Apr 8, 2020 •

edited

Loading

maflcko commented Apr 8, 2020 •

edited by ryanofsky

Loading

ryanofsky commented Apr 8, 2020 •

edited

Loading

laanwj Apr 22, 2020 •

edited

Loading

DrahtBot commented May 13, 2021 •

edited

Loading