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

Define text_object.data.l as "long long" #1768

Merged
merged 2 commits into from
Mar 4, 2024

Conversation

livanh
Copy link
Contributor

@livanh livanh commented Mar 3, 2024

Checklist

  • I have described the changes
  • I have linked to any relevant GitHub issues, if applicable
  • Documentation in doc/ has been updated
  • All new code is licensed under GPLv3

Description

The definition of text_object.data.l is changed from long to long long. In 32-bit environments, long may be as short as 32 bits, and this can cause minor issues. Specifically, I was hitting #1477.

This change solved the issue for me, and it doesn't appear to cause new issues, even on a 64-bit platform. However, it may not be the best fix in the long run, as int and long long are still platform-specific and can hide similar bugs.

Fixed-size definitions (e.g. uint64_t) should be safer, but I'm not sure what the sizes should be (the defaults on x86_64 could be a good starting point). I can update the PR if this solutions turns out to be preferred.

This ensures that text_object.data.l is at least 64 bits in size,
even in a 32-bit environment. It can cause issues if smaller,
such as text turning black instead of a desired color (see brndnmtthws#1477).
@github-actions github-actions bot added the sources PR modifies project sources label Mar 3, 2024
Copy link

netlify bot commented Mar 3, 2024

Deploy Preview for conkyweb canceled.

Name Link
🔨 Latest commit 4a8c8ef
🔍 Latest deploy log https://app.netlify.com/sites/conkyweb/deploys/65e4f0fd18df7600080c6d7d

@brndnmtthws
Copy link
Owner

I suggest using int64_t, less ambiguous and it's part of the C++ standard since C++11.

@brndnmtthws brndnmtthws merged commit ee42a51 into brndnmtthws:main Mar 4, 2024
62 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sources PR modifies project sources
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants