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

[0.4.x] libvisual: fix accidental assignment instead of equality check #351

Open
wants to merge 1 commit into
base: 0.4.x
Choose a base branch
from

Conversation

triallax
Copy link

@triallax triallax commented May 30, 2024

with the accidental assignment, the function would always return 0 for a keytype of VISUAL_HASHMAP_KEY_TYPE_STRING (because it has a value of 2), which is technically valid but not good for obvious reasons

with the accidental assignment, the function would always return 0 for
a keytype of VISUAL_HASHMAP_KEY_TYPE_STRING (because it has a value of
2), which is technically valid but not good for obvious reasons
@triallax triallax force-pushed the fix-accidental-condition-assignment branch from 1b34a3a to 2f46fab Compare May 30, 2024 14:22
@triallax triallax changed the title libvisual: fix accidental assignment instead of equality check [0.4.x] libvisual: fix accidental assignment instead of equality check May 30, 2024
Copy link
Member

@hartwork hartwork left a comment

Choose a reason for hiding this comment

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

@kaixiong what do you think?

@kaixiong
Copy link
Member

kaixiong commented Jun 2, 2024

Nice find. Neither Clang nor GCC warns about this assignment?

@triallax
Copy link
Author

triallax commented Jun 2, 2024

i don't know about gcc, but i found out about this because clang warned about it when i was bumping libvisual for chimera linux:

lv_hashmap.c:264:19: warning: using the result of an assignment as a condition without parentheses [-Wparentheses]
  264 |         else if (keytype = VISUAL_HASHMAP_KEY_TYPE_STRING)
      |                  ~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
lv_hashmap.c:264:19: note: place parentheses around the assignment to silence this warning
  264 |         else if (keytype = VISUAL_HASHMAP_KEY_TYPE_STRING)
      |                          ^                               
      |                  (                                       )
lv_hashmap.c:264:19: note: use '==' to turn this assignment into an equality comparison
  264 |         else if (keytype = VISUAL_HASHMAP_KEY_TYPE_STRING)
      |                          ^
      |                          ==

@kaixiong
Copy link
Member

kaixiong commented Jun 3, 2024

@kaixiong what do you think?

@hartwork would adding -Werror be a good idea? I haven't had to look at or build 0.4.x in a long time to tell.

@hartwork
Copy link
Member

hartwork commented Jun 4, 2024

@kaixiong outside of configure.ac in CI that would be great — yes! —, but my memory says that we're not at zero with warnings and so we cannot enable it without fixing more warnings first. Would need a closer look.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants