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

Clang tidy: round 2 #662

Merged
merged 2 commits into from
Feb 14, 2025
Merged

Clang tidy: round 2 #662

merged 2 commits into from
Feb 14, 2025

Conversation

wismill
Copy link
Member

@wismill wismill commented Feb 13, 2025

CI will pass once #661 is merged.

EDIT: removed clang-tidy CI.

@wismill wismill added ci Continuous integration compiler warning Compiler emits warnings that should be avoided labels Feb 13, 2025
@wismill wismill added this to the 1.9.0 milestone Feb 13, 2025
@wismill wismill requested review from bluetech and whot February 13, 2025 19:48
@wismill wismill force-pushed the clang-tidy/round-2 branch 2 times, most recently from 3717f13 to 56f33fc Compare February 13, 2025 20:18
@wismill
Copy link
Member Author

wismill commented Feb 13, 2025

Well, no chance here. The clang-tidy on the runner seems too old for the current config. Maybe there is an action we can use or a runner with the expected clang-tidy version?

@wismill
Copy link
Member Author

wismill commented Feb 14, 2025

The issue is that my .clang-tidy config works only with the latest clang-tidy, 19.1 (see the release notes). It is not yet available in the Github runners (latest is 18.1 on Ubuntu 24.0.4).

Not sure I want to dig how to make it compatible with earlier versions, or maintain two config versions.

Maybe just deactivate it for now.

@wismill
Copy link
Member Author

wismill commented Feb 14, 2025

Removed the clang-tidy CI, we’ll deal with it in a further PR.

@wismill wismill marked this pull request as ready for review February 14, 2025 06:29
@wismill wismill removed the ci Continuous integration label Feb 14, 2025
@wismill wismill mentioned this pull request Feb 14, 2025
Copy link
Member

@bluetech bluetech left a comment

Choose a reason for hiding this comment

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

LGTM

src/x11/keymap.c Outdated
@@ -197,7 +197,7 @@ translate_action(union xkb_action *action, const xcb_xkb_action_t *wire)
case XCB_XKB_SA_TYPE_SET_GROUP:
action->type = ACTION_TYPE_GROUP_SET;

action->group.group = wire->setgroup.group;
action->group.group = (int32_t) wire->setgroup.group;
Copy link
Member

Choose a reason for hiding this comment

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

IIRC setgroup->group is an int8_t, so why does clang-tidy wants a cast here?

Copy link
Member Author

Choose a reason for hiding this comment

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

The actual warning:

../src/x11/keymap.c:200:31: warning: 'signed char' to 'int32_t' (aka 'int') conversion; consider casting to 'unsigned char' first. [bugprone-signed-char-misuse]
  200 |         action->group.group = wire->setgroup.group;
      |                               ^
../src/x11/keymap.c:213:31: warning: 'signed char' to 'int32_t' (aka 'int') conversion; consider casting to 'unsigned char' first. [bugprone-signed-char-misuse]
  213 |         action->group.group = wire->latchgroup.group;
      |                               ^
../src/x11/keymap.c:226:31: warning: 'signed char' to 'int32_t' (aka 'int') conversion; consider casting to 'unsigned char' first. [bugprone-signed-char-misuse]
  226 |         action->group.group = wire->lockgroup.group;
      |                               ^

From the doc about bugprone-signed-char-misuse:

When the code contains an explicit signed char -> integer conversion, the human programmer probably expects that the converted value matches with the character code (a value from [0..255]), however, the actual value is in [-128..127] interval. To avoid this kind of misinterpretation, the desired way of converting from a signed char to an integer value is converting to unsigned char first, which stores all the characters in the positive [0..255] interval which matches the known character codes.

The issue is that int8_t is just a typedef, so we get this unrelated warning.

Copy link
Member Author

Choose a reason for hiding this comment

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

I added int8_t to the option CharTypdefsToIgnore in .clang-tidy to avoid the explicit cast.

@wismill wismill merged commit c9b0c52 into xkbcommon:master Feb 14, 2025
5 checks passed
@wismill wismill deleted the clang-tidy/round-2 branch February 14, 2025 15:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler warning Compiler emits warnings that should be avoided
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants