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

chore(.clang-tidy): fix settings of readability-identifier-naming #2823

Merged
merged 1 commit into from
Aug 30, 2022

Conversation

kenji-miyake
Copy link
Contributor

@kenji-miyake kenji-miyake commented Aug 29, 2022

Description

The settings that I added in #2763 was a bit wrong.

  • Use _ for PrivateMemberSuffix.
    Google C++ Style Guide says Data members of classes, both static and non-static, are named like ordinary nonmember variables, but with a trailing underscore.
  • Use lower_case with prefix g_ for GlobalConstantCase.
    ROS 2 Docs says For global variables use lowercase with underscores prefixed with g_.
  • Use lower_case for ConstexprVariableCase.
    For these lines.

Pre-review checklist for the PR author

The PR author must check the checkboxes below when creating the PR.

In-review checklist for the PR reviewers

The PR reviewers must check the checkboxes below before approval.

Post-review checklist for the PR author

The PR author must check the checkboxes below before merging.

  • There are no open discussions or they are tracked via tickets.

After all checkboxes are checked, anyone who has write access can merge the PR.

@isamu-takagi
Copy link
Contributor

isamu-takagi commented Aug 29, 2022

Use lower_case for ConstexprVariableCase.

For message type, the rosidl disallows the use of constant names other than UPPER_CASE. Can this pass the check? And the google guideline says to use kConstatntName. So is this Autoware's own rule?

@kenji-miyake
Copy link
Contributor Author

For message type, the rosidl disallows the use of constant names other than UPPER_CASE. Can this pass the check?

I've confirmed this behavior. clang-tidy didn't raise warnings unless we intentionally add -system-headers and -header-fileter=.*.
Since it only affects to user code, I think it's acceptable.

And the google guideline says to use kConstatntName. So is this Autoware's own rule?

Yes. I personally think it's acceptable considering one of the rationales consistency across languages in ROS 2 Docs.

As you write here, it is natural that we use lower_case for constexpr variables.

@isamu-takagi What do you think?

Copy link
Contributor

@isamu-takagi isamu-takagi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Comment on lines 443 to +446
- key: readability-identifier-naming.GlobalConstantCase
value: UPPER_CASE
value: lower_case
- key: readability-identifier-naming.GlobalConstantPrefix
value: g_
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It'll be good to have this finally enforced.

@kenji-miyake kenji-miyake merged commit d04a1b9 into main Aug 30, 2022
@kenji-miyake kenji-miyake deleted the kenji-miyake-patch-1 branch August 30, 2022 07:24
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.

3 participants