p4c
p4c copied to clipboard
Implement the Google variable naming style more faithfully.
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.
Do we want to add abseil-*
checks here?
Do we want to add
abseil-*
checks here?
Sure.
@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?
Classes/Structs/NameSpaces/TypeDefs: PascalCase
Functions/Methods: camelCase
Public/local variables: camelCase
Private variables: camelCase
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?
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.
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.
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.