-
Notifications
You must be signed in to change notification settings - Fork 1
Variable name comments, can't be merged as-is #3
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: gp_CodingConventions
Are you sure you want to change the base?
Conversation
|
|
||
| **[D]** Declaration of identifiers starting with an underscore is **discouraged** | ||
| (even to denote private class members); | ||
| (even to denote private class members); TODO: how about in to name constructor arguments that alias class members? I guess Foo::Foo(int _x) : x(_x) {} in this case |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I use the same namWHHAAAATT??!? err... yes.
Foo::Foo(int x): x{ x } {}I am not going to write that in a recommendation. Anyway, underscore names have earned the fame of being provate class members, so even _x is somehow deceiving. Sometimes I use a Python solution (x_), but I am not particularly proud of that neither. I would be ok to grant an exception for a short-scoped variable like in the example.
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
haha. I'm surprised that works. I know from sad experience that int x = x; is valid syntax and initializes x with garbage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about this other answer to your objection: only private data members should need initialisation in constructors.
Heh?
I am actually not confident of enunciating this as a universal law, but I believe it's reliable in most cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I normally write this pattern for
struct Pt{float x, y; Pt(float _x, float _y) : x(_x), y(_y) {}};
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you trying to force the users to always fully initialise the point? and is it your explicit intention to disable a default constructor?
Otherwise, Pt p { 8.0f, 9.0f }; should work without the constructor you described:
struct Pt { float x, y; };I don't want to focus too much on the specific example, but the questions are general.
Edit: according to the current guidelines, that structure would look like:
/// 2D coordinates [cm]
struct Pt {
float x { 0.0 }, y { 0.0 };
};There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Partly it's because I'm an old fuddy-duddy who prefers parenthesis initialization to brace initialization. But also, you don't have to get much more complicated before the ability to have a default parameter, or two constructors taking different arguments, becomes useful.
|
|
||
| **[R]** Use of a descriptive control variable is **required** | ||
| in any loop longer than five lines. | ||
| in any loop longer than five lines, unless implementing a mathematical formula (matrix operations?) where i,j,k indices are traditional |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Somehow in a matrix operation I do consider i, j and k descriptive, or mu, nu, lambda, rhofor tensors, andx, yandz` when there is only one vector around. Do you thing we need to spell an exception out?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm worried the original suggestion will have people calling their variables firstMatrixSubscript and so on, so I think it's probably better to be explicit (though could combine with the part where you talk about physics equations)
sbn/codingconv/CodingConventions.md
Outdated
| `double const F = G * m1 * m2 / (d*d);` should still be preferred. | ||
|
|
||
| **[E]** Starting private data members with `f` and use CamelCase is **encouraged** (e.g. `double fTrackLength`). | ||
| **[E]** Starting private data members with `f` and use CamelCase is **encouraged** (e.g. `double fTrackLength`). Agree, but can we spell this out more / ban starting non-members with `f`. Lots of people seem to be confused about this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about:
| **[E]** Starting private data members with `f` and use CamelCase is **encouraged** (e.g. `double fTrackLength`). Agree, but can we spell this out more / ban starting non-members with `f`. Lots of people seem to be confused about this. | |
| **[E]** Names starting with `f` and using CamelCase (e.g. `double fTrackLength`) are **encouraged** for private data members, and only for them. Conversely, the use of the trailing `f` for local variables is **discouraged**. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"leading", not "trailing" f, and I don't think you want to discourage camel case for local variables.
Maybe this is clearer with examples: ClassName, FunctionName(), fMemberVar, localVar etc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like we need a example table (plus the load of examples throughout the test, if consistent).
I'll settle for this text and then schedule a table.
| **[E]** Starting private data members with `f` and use CamelCase is **encouraged** (e.g. `double fTrackLength`). Agree, but can we spell this out more / ban starting non-members with `f`. Lots of people seem to be confused about this. | |
| **[E]** Names starting with `f` and using CamelCase (e.g. `double fTrackLength`) are **encouraged** for private data members, and only for them. Conversely, the use of a leading `f` for local variables is **discouraged**. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you still write that camel case is only for private members. Are you calling thisStyle for local variables by a different name? We might need to clarify the jargon in a table / with examples as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I call thisStile drinking camelcase and ThisStyle staring camelcase. I am sure that nomenclature is universal and internationally acclaimed.
But to your point, I get it: you prefer the full rule spelled out.
**[E]** Private data member names are **encouraged** to start with `f` and use CamelCase
(e.g. `double fTrackLength`). This pattern should be reserved exclusively for such private data member.
Conversely, public data members and local variables should instead follow the more general guideline
expressed above (i.e. simple camelCase, e.g. `trackLength`).
bb2c237 to
add850e
Compare
No description provided.