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

Avoid reserved identifiers #58200

Merged

Conversation

jbytheway
Copy link
Contributor

@jbytheway jbytheway commented Jun 6, 2022

Summary

None

Purpose of change

More static analysis. These three checks look for use of reserved identifiers.

Describe the solution

There are three clang-tidy checks which are looking for use of reserved identifiers. Enable all of them and fix or suppress the associated warnings.

The reserved identifiers we had fall into three categories:

  • Names containing __
  • Names starting with _ followed by a capital letter.
  • Names starting with _ in the global namespace.

Most of those could be automatically fixed by clang-tidy.

One was also affected by the cata-static-string_id-constants check, which needed to be updated so they wouldn't conflict.

Two cases were erroneous (looks like a clang-tidy bug because it was reporting the wrong identifiers) and needed suppression.

_USE_MATH_DEFINES is an intentional definition of a library macro, and needed suppression.

The _ macro we use for translation is a reserved identifier, but we're going to use it anyway. So that is also suppressed.

Describe alternatives you've considered

Not worrying about these names.

Looking into the buggy reports more carefully to see what was up with them.

Testing

There's a small chance this change has introduced a name collision and thus some bug, but I think that probably would have led to a compiler error.

Additional context

There are three clang-tidy checks which are looking for use of reserved
identifiers.  Enable all of them and fix or suppress the associated
warnings.

The reserved identifiers we had fall into three categories:
- Names containing "__"
- Names starting with "_" followed by a capital letter.
- Names starting with "_" in the global namespace.

Most of those could be automatically fixed by clang-tidy.

One was also affected by the cata-static-string_id-constants check,
which needed to be updated so they wouldn't conflict.

Two cases were erroneous and needed suppression.

The _ macro we use for translation is a reserved identifier, but we're
going to use it anyway.  So that is also suppressed.
@jbytheway jbytheway requested a review from KorGgenT as a code owner June 6, 2022 01:06
@anothersimulacrum anothersimulacrum added [C++] Changes (can be) made in C++. Previously named `Code` Code: Infrastructure / Style / Static Analysis Code internal infrastructure and style labels Jun 6, 2022
@github-actions github-actions bot added Code: Tests Measurement, self-control, statistics, balancing. Code: Tooling Tooling that is not part of the main game but is part of the repo. Info / User Interface Game - player communication, menus, etc. json-styled JSON lint passed, label assigned by github actions astyled astyled PR, label is assigned by github actions BasicBuildPassed This PR builds correctly, label assigned by github actions labels Jun 6, 2022
@ZhilkinSerg ZhilkinSerg merged commit 14a6293 into CleverRaven:master Jun 6, 2022
@jbytheway jbytheway deleted the clang-tidy_reserved-identifier branch June 6, 2022 23:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
astyled astyled PR, label is assigned by github actions BasicBuildPassed This PR builds correctly, label assigned by github actions [C++] Changes (can be) made in C++. Previously named `Code` Code: Infrastructure / Style / Static Analysis Code internal infrastructure and style Code: Tests Measurement, self-control, statistics, balancing. Code: Tooling Tooling that is not part of the main game but is part of the repo. Info / User Interface Game - player communication, menus, etc. json-styled JSON lint passed, label assigned by github actions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants