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.

Sign up for GitHub

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

Fix warnings on 32-bit platforms #159

Merged
merged 1 commit into from
Jul 17, 2024
+16 −13
Merged

Fix warnings on 32-bit platforms #159

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 5 additions & 4 deletions src/config.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@ static bool load_config(const char* path)
++line;
delim = strchr(line, ']');
if (!delim || line + 1 == delim) {
fprintf(stderr, "Invalid section define in %s:%lu\n", path,
fprintf(stderr, "Invalid section define in %s:%zu\n", path,
line_num);
continue;
}
Expand All @@ -132,7 +132,7 @@ static bool load_config(const char* path)

delim = strchr(line, '=');
if (!delim) {
fprintf(stderr, "Invalid key=value format in %s:%lu\n", path,
fprintf(stderr, "Invalid key=value format in %s:%zu\n", path,
line_num);
continue;
}
Expand All @@ -151,7 +151,7 @@ static bool load_config(const char* path)
// load configuration parameter from key/value pair
status = config_set(section, line, value);
if (status != cfgst_ok) {
fprintf(stderr, "Invalid configuration in %s:%lu\n", path,
fprintf(stderr, "Invalid configuration in %s:%zu\n", path,
line_num);
}
}
Expand Down Expand Up @@ -299,7 +299,8 @@ bool config_to_color(const char* text, argb_t* color)
++text;
}

if (!str_to_num(text, 0, &num, 16) || num < 0 || num > 0xffffffff) {
if (!str_to_num(text, 0, &num, 16) || num < 0 ||
(uint64_t)num > (uint64_t)0xffffffff) {
Copy link
Owner

Choose a reason for hiding this comment

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

Do we really need these type casts?
0xffffffff fits uint32_t. Also color is always 32 bits.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On 32-bit systems, 0xffffffff will be considered a 4-byte unsigned int by compilers, as it can't fit in an int without overflowing. Also, ssize_t num will be a 4-byte int (not guaranteed by the C spec, but what compilers usually do). The compiler will emit a warning because these two different types are being compared, and the compiler doesn't know whether it should generate a signed or unsigned comparison. The cast to uint64_t assures that these two values can be safely compared on both 32-bit and 64-bit systems. On 64-bit systems, because this code checks for the validity of a color, num can be greater than 32-bits (it's inputted by the user in the configuration file). A cast of num to uint32_t wouldn't suffice, as values higher than 0xffffffff would be accepted as valid colors, as they'd be truncated before the comparison.

return false;
}

Expand Down
8 changes: 4 additions & 4 deletions src/info.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -397,14 +397,14 @@ void info_update(size_t frame_idx, float scale)
(ctx.frame != frame_idx || ctx.frame_total != image->num_frames)) {
ctx.frame = frame_idx;
ctx.frame_total = image->num_frames;
snprintf(buffer, sizeof(buffer), "%lu of %lu", ctx.frame + 1,
snprintf(buffer, sizeof(buffer), "%zu of %zu", ctx.frame + 1,
ctx.frame_total);
update_field(buffer, &ctx.fields[info_frame].value);
}

if (is_visible(info_index) && ctx.index != loader_current_index()) {
ctx.index = loader_current_index();
snprintf(buffer, sizeof(buffer), "%lu of %lu", ctx.index + 1,
snprintf(buffer, sizeof(buffer), "%zu of %zu", ctx.index + 1,
image_list_size());
update_field(buffer, &ctx.fields[info_index].value);
}
Expand All @@ -413,7 +413,7 @@ void info_update(size_t frame_idx, float scale)
const size_t scale_percent = scale * 100;
if (ctx.scale != scale_percent) {
ctx.scale = scale_percent;
snprintf(buffer, sizeof(buffer), "%ld%%", ctx.scale);
snprintf(buffer, sizeof(buffer), "%zu%%", ctx.scale);
update_field(buffer, &ctx.fields[info_scale].value);
}
}
Expand All @@ -423,7 +423,7 @@ void info_update(size_t frame_idx, float scale)
if (ctx.width != pm->width || ctx.height != pm->height) {
ctx.width = pm->width;
ctx.height = pm->height;
snprintf(buffer, sizeof(buffer), "%lux%lu", ctx.width, ctx.height);
snprintf(buffer, sizeof(buffer), "%zux%zu", ctx.width, ctx.height);
update_field(buffer, &ctx.fields[info_image_size].value);
}
}
Expand Down
8 changes: 4 additions & 4 deletions src/ui.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,8 +157,8 @@ static struct wl_buffer* create_buffer(void)

// generate unique file name
clock_gettime(CLOCK_MONOTONIC, &ts);
snprintf(path, sizeof(path), "/" APP_NAME "_%lx",
(ts.tv_sec << 32) | ts.tv_nsec);
snprintf(path, sizeof(path), "/" APP_NAME "_%" PRIx64,
Copy link
Owner

Choose a reason for hiding this comment

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

It would be better to use one style for printf.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, but I don't know what you mean by this. PRIx64 is the best way to print an uint64_t independently of the size unsigned long ("%lx") or unsigned long long ("%llx") might have. Another option would be casting the value in the arguments to an unsigned long long, and %llx can be used instead of PRIx64 . I can patch that in if you wish.

((uint64_t)ts.tv_sec << 32) | ts.tv_nsec);

// open shared mem
fd = shm_open(path, O_RDWR | O_CREAT | O_EXCL, 0600);
Expand Down Expand Up @@ -698,8 +698,8 @@ void ui_create(void)
// create unique application id
if (clock_gettime(CLOCK_MONOTONIC, &ts) == 0) {
char app_id[64];
const uint64_t timestamp = (ts.tv_sec << 32) | ts.tv_nsec;
snprintf(app_id, sizeof(app_id), APP_NAME "_%lx", timestamp);
const uint64_t timestamp = ((uint64_t)ts.tv_sec << 32) | ts.tv_nsec;
snprintf(app_id, sizeof(app_id), APP_NAME "_%" PRIx64, timestamp);
str_dup(app_id, &ctx.app_id);
} else {
str_dup(APP_NAME, &ctx.app_id);
Expand Down
4 changes: 3 additions & 1 deletion src/ui.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@

#include "pixmap.h"

#include <limits.h>

// Configuration parameters
#define UI_CFG_APP_ID "app_id"
#define UI_CFG_FULLSCREEN "fullscreen"
Expand All @@ -15,7 +17,7 @@
// Special ids for windows size and position
#define SIZE_FROM_IMAGE 0
#define SIZE_FROM_PARENT 1
#define POS_FROM_PARENT 0xffffffff
#define POS_FROM_PARENT SSIZE_MAX
Copy link
Owner

Choose a reason for hiding this comment

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

I don't see a reason to change this. What the purpose?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

0xffffffff may be too large for an ssize_t, and some functions (ui_get_x and ui_get_y) return this value as an ssize_t. I see that POS_FROM_PARENT is a special return value: normal positions can be returned, so can this very large value, which has a special meaning. This way, SSIZE_MAX is for sure the greatest value that can fit in an ssize_t.


/**
* Create User Interface context.
Expand Down
Loading