[clang-tidy] Vote: Naming style

j-stephan commented 4 years ago

Since clang-tidy offers countless options that can be used for checking the code base I believe it makes sense to split them into several groups. These groups will then be voted on as whole in order to speed up the process.

This PR deals with our naming style. To make it easier to interpret this is what the current proposal will change:

All macros will be written in UPPER_CASE
Everything that is used as parameter inside a template <> expression will be written in Camel_Snake_Case
Everything else will be written in lower_case (a.k.a. snake_case)

I chose this naming style to be more aligned with the C++ ecosystem style (standard library, boost, ...). I would have preferred to write template parameters as Slightly_different_snake_case but unfortunately clang-tidy does not offer this option.

Since there should be enough time to view all the different options (see description here) this PR will be closed in two weeks (meaning Tuesday, 03 November 2020, 18:00 HZDR time) - the vote will then be final.

Use the following template for voting:

Option | `lower_case` | `UPPER_CASE` |  `camelBack` | `CamelCase` | `camel_Snake_Back` | `Camel_Snake_Case` | `aNy_CasE` |
-------|--------------|--------------|--------------|-------------|--------------------|--------------------|------------|
default | 0 | 0 | 0 | 0 | 0 | 0 | 0 |
Macros | 0 | 0 | 0 | 0 | 0 | 0 | 0 |
Template parameters | 0 | 0 | 0 | 0 | 0 | 0 | 0 |

Feel free to add specific options to the vote. If you don't care about a particular option don't increase its counter.

Option	`lower_case`	`UPPER_CASE`	`Camel_Snake_Case`
default	1	0	0
Macros	0	1	0
Template parameters	0	0	1

bernhardmgruber commented 4 years ago

I think having everything except macros and template parameters have the same case is a severe restriction. The check itself supports ample configuration options that we could specify, or just leave them unspecified. I do not think we need to nail down everything. We could just pick the most important 5 items and leave the rest.

Option	`lower_case`	`UPPER_CASE`	`camelBack`	`CamelCase`	`camel_Snake_Back`	`Camel_Snake_Case`	`aNy_CasE`
Macros	0	2	0	0	0	0	0
Template parameters	0	0	0	1	0	1	0

jkelling commented 4 years ago

This table appears to be missing type names. Currently PIConGPU and Alpaka use CamelCase for type names and camelBack for other identifiers. Are both subsumed under "default" in this table? Is there a way to split this up?

I prefer variations of camelCase (no underscores) because it is easier on the eye. Macros on the other hand should have underscores, they need to look ugly.

j-stephan commented 4 years ago

Are both subsumed under "default" in this table?

Right now they are. "Default" should be interpreted in a "catch-all" way, meaning that it covers all options that haven't been explicitly mentioned.

Is there a way to split this up?

Yes. Feel free to add new rows to the vote covering the cases you want to take out of the "default" set. See the documentation for all the options that are available: https://clang.llvm.org/extra/clang-tidy/checks/readability-identifier-naming.html

jkelling commented 4 years ago

Option	lower_case	UPPER_CASE	camelBack	CamelCase	Camel_Snake_Case
default	1	0	1	0	0
Macros	0	3	0	0	0
Template parameters	0	0	0	2	1
ClassCase	1	0	0	1	0

j-stephan commented 4 years ago

Suggestion: If you add a new line be sure to derive the according votes from the "default" set. In the example above there would be a lower_case vote for ClassCase. Otherwise everyone has to constantly update their votes whenever there is a new line.

Sorry for not mentioning this earlier, the problem only occurred to me after your vote.

psychocoderHPC commented 4 years ago

Option	lower_case	UPPER_CASE	camelBack	CamelCase	Camel_Snake_Case
default	2	0	1	0	0
Macros	0	4	0	0	0
Template parameters	0	0	0	3	1
ClassCase	1	0	0	1	1

I am a big fan of camel case but for types I see that _t and for variables _v like in std is very nice.

jkelling commented 4 years ago

@psychocoderHPC There dedicated options for prefixes and suffixes. We could vote on e.g. ClassSuffix = _t and ClassMemberPrefix = m_ vs. ClassMemberSuffix = _ (I grew up with the Qt/KDE style, so I am probalby opposed to the STL style in most cases). This is orthogonal to the case option.

bernhardmgruber commented 4 years ago

Option	lower_case	UPPER_CASE	camelBack	CamelCase	Camel_Snake_Case
default	2	0	2	0	0
Macros	0	4	0	0	0
Template parameters	0	0	0	3	1
ClassCase	1	0	0	2	1
EnumCase	2	0	1	1	0
TypeAliasCase	2	0	1	1	0
UnionCase	2	0	1	1	0

bernhardmgruber commented 4 years ago

I am a big fan of camel case but for types I see that _t and for variables _v like in std is very nice.

I can't wait to see you write:

for (int i_v = 0; i_v < n_v; i_v++) { ... }

:D

SimeonEhrig commented 4 years ago

Option	`lower_case`	`UPPER_CASE`	`camelBack`	`CamelCase`	`Camel_Snake_Case`
default	3	0	2	0	0
Macros	0	5	0	0	0
Template parameters	0	0	0	4	1
ClassCase	1	0	0	3	1
EnumCase	2	0	1	2	0
TypeAliasCase	2	0	1	2	0
UnionCase	2	0	1	2	0

jkelling commented 4 years ago

I am a big fan of camel case but for types I see that _t and for variables _v like in std is very nice.

I can't wait to see you write:
for (int i_v = 0; i_v < n_v; i_v++) { ... }

I see both your points: The _t is more explicit, but if you mark all things with some suffix or prefix, your are marking one thing too many.

sbastrakov commented 3 years ago

I think @psychocoderHPC meant only for type traits and metaprogramming things like how the standard library names things: X_t<...> == typename X<...>::type, X_v<...> == X<...>::value. Not for normal type and variable names.

sbastrakov commented 3 years ago

Option	lower_case	UPPER_CASE	camelBack	CamelCase	Camel_Snake_Case
default	4	0	2	0	0
Macros	0	6	0	0	0
Template parameters	1	0	0	4	1
ClassCase	2	0	0	3	1
EnumCase	3	0	1	2	0
TypeAliasCase	3	0	1	2	0
UnionCase	3	0	1	2	0

bernhardmgruber commented 3 years ago

So we are going to have classes spelled with CamelCase but enums, aliases and unions with lower_case? That seems wrong to me.

jkelling commented 3 years ago

So we are going to have classes spelled with CamelCase but enums, aliases and unions with lower_case? That seems wrong to me.

@bernhardmgruber you are the one who split the CamelCase vote to chose camelBack for enums, aliases and unions. I presume these setting apply to the enum/alias/union type name not to members (would make not sense otherwise for aliases). Maybe you should reconsider your choice ;) .

bernhardmgruber commented 3 years ago

@bernhardmgruber you are the one who split the CamelCase vote to chose camelBack for enums, aliases and unions. I presume these setting apply to the enum/alias/union type name not to members (would make not sense otherwise for aliases). Maybe you should reconsider your choice ;) .

This is not how I understood the vote to work. I did NOT choose camelBack in my vote for enums, type aliases and unions. I added 3 rows to the table, so I must duplicate all values from the default row. Since there was 1 vote for camelBack at default (by @jkelling), I must duplicate these into the 3 new rows. Therefore, @jkelling now votes for camelBack on enums, type aliases and unions. Then I added my vote for CamelCase on these 3 new rows. And I added my vote to the default row for camelBack.

In general, I think these copying of the previously voted on defaults is wrong. It punishes new options added later to the vote.

Example: I just found out, that ClassName does not affect struct names. So I will add it. Copying the defaults now (-1, because I remove my default) gives us:

Option	lower_case	UPPER_CASE	camelBack	CamelCase	Camel_Snake_Case
default	4	0	2	0	0
Macros	0	6	0	0	0
Template parameters	1	0	0	4	1
ClassCase	2	0	0	3	1
EnumCase	3	0	1	2	0
TypeAliasCase	3	0	1	2	0
UnionCase	3	0	1	2	0
StructCase	4	0	1	1	0

And now if not all of the people who voted already reconsider, we might have an unwanted outcome.

j-stephan commented 3 years ago

we might have an unwanted outcome.

Depends on whom you ask ;-) I assume that everyone who cares monitors this PR and adjusts their vote as needed. But to be honest I also assume some sensible extensions to the vote. For example I would have implicitly set StructCase / AbstractClassCase to ClassCase's value. Or made template <typename Parameter>, template<template <typename Parameter>> and template <int Parameter> the same case.

j-stephan commented 3 years ago

Since these objections are kind-of last minute: Shall we extend the vote or go with the current results? Pinging @psychocoderHPC @sbastrakov @SimeonEhrig

jkelling commented 3 years ago

Along the line of what @j-stephan wrote, I assumed, that the case of everything which is a type would also be ClaseCase, if not stated otherwise. So I thought @bernhardmgruber derived my votes for UnionCase/StructCase/TypeAliasCase from the ClassCase vote as the enclosing group, and himself voted for camelCase on those. Let me fix this then:

Option	lower_case	UPPER_CASE	camelBack	CamelCase	Camel_Snake_Case
default	4	0	2	0	0
Macros	0	6	0	0	0
Template parameters	1	0	0	4	1
ClassCase	2	0	0	3	1
EnumCase	3	0	0	3	0
TypeAliasCase	3	0	0	3	0
UnionCase	3	0	0	3	0
StructCase	4	0	0	2	0

It seems the same thing happened in the StructCase post? Someone who voted for ClassCase=CamelCase had default=lower_case and thus got their vote for StructCase set to lower_case? Is there anyone who actively wants CamelCase for classes and lower_case for struct at the same time? Maybe it would be better to have everyone update their vote themselves when new rows are added to avoid confusion.

Regarding the vote:

In case there is no absolute majority on a setting, there should be a new vote with all 0 columns and the lowest non-zero column removed. Repeat until one column gets an absolute majority. First-past-the-post is bad.
As it does not appear to be clear to everyone what we are effectively voting on, due to differing assumptions about implied votes, we should look at a code example of the outcome of this vote and have another vote.

psychocoderHPC commented 3 years ago

Since these objections are kind-of last minute: Shall we extend the vote or go with the current results? Pinging @psychocoderHPC @sbastrakov @SimeonEhrig

Yes please give us two more days to vote also for the added options.

psychocoderHPC commented 3 years ago

Option	lower_case	UPPER_CASE	camelBack	CamelCase	Camel_Snake_Case
default	4	0	2	0	0
Macros	0	6	0	0	0
Template parameters	1	0	0	4	1
ClassCase	2	0	0	3	1
EnumCase	3	0	0	3	1
TypeAliasCase	3	0	0	3	1
UnionCase	3	0	0	3	1
StructCase	4	0	0	2	1

I added my votes for the last 4 (new) options

jkelling commented 3 years ago

I added my votes for the last 4 (new) options

@psychocoderHPC You need to also remove your votes from the lower_case columns, because your default was lower_case. Currently, the new rows have seven votes in them, while only six people voted.

I guess we need to count from the start or start over.

j-stephan commented 3 years ago

Vote extended until 06 November 2020, 18:00 HZDR time.

j-stephan commented 3 years ago

Cleaned vote count for better visibility:

Option	`lower_case`	`UPPER_CASE`	`camelBack`	`CamelCase`	`Camel_Snake_Case`
default	4	0	2	0	0
Macros	0	6	0	0	0
Template parameters	1	0	0	4	1
ClassCase	2	0	0	3	1
EnumCase	2	0	0	3	1
TypeAliasCase	2	0	0	3	1
UnionCase	2	0	0	3	1
StructCase	3	0	0	2	1

Remember that voting will be closed today at 18:00. @bernhardmgruber @jkelling @SimeonEhrig @psychocoderHPC @sbastrakov

bernhardmgruber commented 3 years ago

Could the person that voted for CamelCase classes but lower_case structs please reconsider? Who is that btw?

jkelling commented 3 years ago

I agree with @bernhardmgruber and doubt that all votes (specifically that one) were counted in accordance with the voter's intend. The probably actual vote count is likely:

Option	lower_case	UPPER_CASE	camelBack	CamelCase	Camel_Snake_Case
default	4	0	2	0	0
Macros	0	6	0	0	0
Template parameters	1	0	0	4	1
AnyTypeName \ TemplateParams	2	0	0	3	1

j-stephan commented 3 years ago

Who is that btw?

I think that was (unwillingly) you. Looks like your example vote above got copied around.

bernhardmgruber commented 3 years ago

Who is that btw?

I think that was (unwillingly) you. Looks like your example vote above got copied around.

I ran through all the votes again and that is what I end up with:

lower_case class: jan sergei lower_case struct: jan sergei simeon CamelCase class: jeffrey bgruber simeon CamelCase struct: jeffrey bgruber

Because Simeon never changed his vote after we added the row with StructCase. @SimeonEhrig could you please reconsider?

j-stephan commented 3 years ago

Simeon isn't working today. I'll extend the vote again until Tuesday, 18:00 to give him the chance to reconsider.

jkelling commented 3 years ago

@bernhardmgruber @j-stephan you two should agree on what default means. According to @bernhardmgruber @SimeonEhrig meant to say: "classes should look differently from structs" when he cast his ClassCase vote. According to @j-stephan @SimeonEhrig should have assumed, that voting on ClassCase implied to vote the same way on StructCase, in which case he does not need to reconsider, but move his vote to the correct column.

I think it would be best to remove people's votes on new columns, which they did not explicitly vote on, i.e have them abstain on these until they update their votes. It is probably better to abstain, then have votes counted counter to their intention.

bernhardmgruber commented 3 years ago

I think it would be best to remove people's votes on new columns, which they did not explicitly vote on, i.e have them abstain on these until they update their votes. It is probably better to abstain, then have votes counted counter to their intention.

Absolutely in favor! We should do that in the future! For this poll, it breaks @j-stephan's request: https://github.com/ComputationalRadiationPhysics/contributing/pull/42#issuecomment-714348325

SimeonEhrig commented 3 years ago

I'm totally confused. Is it enough, If I say, that want to write class and struct names both in CamelCase?

j-stephan commented 3 years ago

Yes. This would leave us the following votes:

Option	`lower_case`	`UPPER_CASE`	`camelBack`	`CamelCase`	`Camel_Snake_Case`
default	4	0	2	0	0
Macros	0	6	0	0	0
Template parameters	1	0	0	4	1
ClassCase	2	0	0	3	1
EnumCase	2	0	0	3	1
TypeAliasCase	2	0	0	3	1
UnionCase	2	0	0	3	1
StructCase	2	0	0	3	1

Is everyone happy with this being the final result?

@sbastrakov @SimeonEhrig @bernhardmgruber @psychocoderHPC @jkelling

bernhardmgruber commented 3 years ago

I am happy with the results we voted on so far. I still undertstand your statement

Right now they are. "Default" should be interpreted in a "catch-all" way, meaning that it covers all options that haven't been explicitly mentioned.

as: We can still vote on further options at a later stage. But lower_case will be the default for everything not mentioned so far. Is this correct?

j-stephan commented 3 years ago

But lower_case will be the default for everything not mentioned so far. Is this correct?

Indeed.

ComputationalRadiationPhysics / contributing

[clang-tidy] Vote: Naming style #42