-
Notifications
You must be signed in to change notification settings - Fork 24
Name constants with a leading "k" and mixed case #140
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: master
Are you sure you want to change the base?
Conversation
A C++ programming convention, also suggested by the [Google style guide](https://google.github.io/styleguide/cppguide.html#Constant_Names) and enforced by `cpplint`, is to name variables declared `constexpr` or `const`, and whose value is fixed for the duration of the program, by a leading "k" followed by mixed case. There are only two files that require changes, one in the main source, which may require some discussion, and one in the test suite, which is uncontroversial, I think. This commit fixes both of them.
lundgren87
left a comment
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've mentioned one case inline as an example, but in general I am not sure why this should not apply to all static/global const-declared variables in that case?
I think there are quite a few more that violate this code standard should we adopt it (which is perhaps the real question?).
| constexpr int nThreads = 16; | ||
| constexpr int kThreads = 16; | ||
| /** @brief number of itereations to run for some of the tests */ | ||
| constexpr int iter = 10000; |
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 fail to understand what the difference between L33 and L35 is?
It seems to me that if one adopts the kMixedCase convention, should this not apply to both?
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 have a point there.
My guess is that the cpplint check(s) concern only variables that are used as array indices and are either in camel case (e.g. nThreads) or in snake case (e.g. page_info_size). The checks leave variables such as iter which are not used as array indices unchanged.
|
My guess is that the NB: In case you did not notice this already, the |
|
Does the change from
|
A C++ programming convention, also suggested by the Google style guide and enforced by
cpplint, is to name variables declaredconstexprorconst, and whose value is fixed for the duration of the program, by a leading "k" followed by mixed case.There are only two files that require changes, one in the main source, which may require some discussion, and one in the test suite, which is uncontroversial, I think. This commit fixes both of them.