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

Adjusting GDCLASS macro to modern C++ clang-tidy checks #1310

Closed

Conversation

grabusr
Copy link

@grabusr grabusr commented Nov 15, 2023

Changes

  1. Using modern rule of 5: forbidding moving and copying of class instead of making assignment operator private.
  2. Using auto to avoid type repetition.

Why

Using GDCLASS is not treated by clang-tidy as usage of system header resource, because it is macro, so it perform checks on it. To make possible pass code inspection by clang-tidy checks (modernize-use-auto and cppcoreguidelines-special-member-functions) given change are needed.

@grabusr grabusr requested a review from a team as a code owner November 15, 2023 18:34
@grabusr grabusr changed the title Adjusting GDCLASS macro to modern-cpp clang-tidy checks Adjusting GDCLASS macro to modern C++ clang-tidy checks Nov 15, 2023
@@ -238,7 +243,7 @@ public:
static void notification_bind(GDExtensionClassInstancePtr p_instance, int32_t p_what, GDExtensionBool p_reversed) { \
if (p_instance && m_class::_get_notification()) { \
if (m_class::_get_notification() != m_inherits::_get_notification()) { \
m_class *cls = reinterpret_cast<m_class *>(p_instance); \
auto cls = reinterpret_cast<m_class *>(p_instance); \
Copy link
Member

Choose a reason for hiding this comment

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

auto is not allowed

That should apply here as well, no reason to use it here

@AThousandShips AThousandShips added enhancement This is an enhancement on the current functionality discussion labels Nov 15, 2023
@grabusr
Copy link
Author

grabusr commented Nov 15, 2023

Ok, then I need to some workaround to disable clang-tidy check for that part.

@AThousandShips
Copy link
Member

You should always adapt your the use of your tool to fit the codebase, not the other way around

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
archived enhancement This is an enhancement on the current functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants