p4c icon indicating copy to clipboard operation
p4c copied to clipboard

Implement the Google variable naming style more faithfully.

Open fruffy opened this issue 1 year ago • 7 comments

Fixes #4404.

The closest approximation of the Google style I could find is defined here: https://github.com/googleapis/google-cloud-cpp/blob/main/.clang-tidy#L104

We deviate quite a bit. For example, we use camelCase in most places instead of CamelCase and snake_case for variables. I do not have a strong opinion on the style except that we should decide for one and stick to it. These days clang-tidy is advanced enough to actually enforce it, which is nice.

Pinging some people on this PR to get agreement. This will be helpful in reviews going forward.

fruffy avatar Feb 11 '24 21:02 fruffy

Do we want to add abseil-* checks here?

asl avatar Apr 05 '24 19:04 asl

Do we want to add abseil-* checks here?

Sure.

fruffy avatar Apr 05 '24 19:04 fruffy

@ChrisDodd

With type you mean a typedef?

typedef or nested struct

Unfortunately, there is no way to distinguish between a private and public typedef for now. But we could add the _t suffix.

However, the current configuration here uses the prefix style. So _camelCase for private member variables instead of lower_case. This might be more consistent than snake case for only private members?

This is what it would look like:

Classes/Structs/NameSpaces/TypeDefs: PascalCase
Functions/Methods: camelCase
Public/local variables: camelCase
Private variables: _camelCase

For reference, other styles: Google C++ style is:

Classes/Structs/NameSpaces/TypeDefs: PascalCase
Functions/Methods: PascalCase
Public/local variables: snake_case
Private variables: _snake_case?

C# style is:

Classes/Structs/NameSpaces/TypeDefs: PascalCase
Functions/Methods: PascalCase
Public/local variables: PascalCase
Private variables: _camelCase?

Java Style

Classes/Structs/NameSpaces/TypeDefs: PascalCase
Functions/Methods: camelCase
Public/local variables: camelCase
Private variables: camelCase

fruffy avatar May 01 '24 23:05 fruffy

I don't have many strong opinions about naming style. Personally I find _t suffix annoying and prefer uppercase for types which matches with the proposed style. But mainly I am for naming consistency.

Do you prefer to enforce this (by a CI check)?

Did you try how large changes are needed to make the codebase conform?

vlstill avatar May 02 '24 10:05 vlstill

Do you prefer to enforce this (by a CI check)?

We are still missing Clang-tidy support for this (https://github.com/p4lang/p4c/pull/4254).

Did you try how large changes are needed to make the codebase conform?

There are quite a few changes needed across the codebase, but I'd say we can just gradually fix them. Just having the clang-tidy standards will encourage developers to use the right naming scheme.

fruffy avatar May 02 '24 15:05 fruffy

Do you prefer to enforce this (by a CI check)?

We are still missing Clang-tidy support for this (#4254).

Would the clang-tidy still run very slowly if you disabled all the checks except for the formatting once? Maybe then it would be manageable.

Of course, if we wanted enforce it we would also have to re-format whole codebase at once.

vlstill avatar May 02 '24 15:05 vlstill

Would the clang-tidy still run very slowly if you disabled all the checks except for the formatting once?

Yes, I have not found a way yet. But even having it as part of CMakelists is already useful for cleanup, e.g., https://github.com/p4lang/p4c/pull/4326

Of course, if we wanted enforce it we would also have to re-format whole codebase at once.

I do not think this is necessary, we can do this gradually. Unlike clang-format these changes are more sensitive.

fruffy avatar May 02 '24 15:05 fruffy