Skip to content
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

Name constants with a leading "k" and mixed case #140

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kostis
Copy link
Contributor

@kostis kostis commented Nov 29, 2022

A C++ programming convention, also suggested by the Google style guide 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.

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.
Copy link
Member

@lundgren87 lundgren87 left a 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?).

@@ -30,7 +30,7 @@ constexpr std::size_t size = 1<<20;
constexpr std::size_t cache_size = size;

/** @brief number of threads to spawn for some of the tests */
constexpr int nThreads = 16;
constexpr int kThreads = 16;
/** @brief number of itereations to run for some of the tests */
constexpr int iter = 10000;
Copy link
Member

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?

@kostis
Copy link
Contributor Author

kostis commented Dec 2, 2022

My guess is that the cppcheck rule only applies to variables either containing underscores or already in CamelCase but leaves simple variables (e.g. i, n, iter, ...) without complaints.

NB: In case you did not notice this already, the cppcheck rule applies only to variables used in array element accesses, not all variables (so the iter variable is not a good example here).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants