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

Fix off_t and size_t printf warnings on 32/64 bit systems. #26

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mbroz
Copy link
Contributor

@mbroz mbroz commented Apr 30, 2024

Dieharder already uses _FILE_OFFSET_BITS=64, so off_t type is always 64bit signed integer.

However, on 32bit platforms is is presented as long long, while on 64bit it is just long (as long type is 64bit there).

As there is no specific format prefix for off_t, IMO, the safest is to use integer promotion - use larges type prefix %lld and cast value to (long long) in printf.

Also switch printf to signed integer, as off_t is always signed.

For size_t we have the same issue, but here we have already print prefix %zu that will handle size_ on all patforms.

This should replace some part of #25 except sscanf issues.

Dieharder already uses _FILE_OFFSET_BITS=64, so off_t type
is always 64bit signed integer.

However, on 32bit platforms is is presented as long long,
while on 64bit it is just long (as long type is 64bit there).

As there is no specific format prefix for off_t, IMO, the safest
is to use integer promotion - use larges type prefix %lld
and cast value to (long long) in printf.

Also switch printf to signed integer, as off_t is always signed.

For size_t we have the same issue, but here we have already
print prefix %zu that will handle size_ on all patforms.
@eddelbuettel
Copy link
Owner

I think this goes in the right direction but it is 2024 and I am not a fan of propagating long long with the known-different-size-on-different-platforms.

Can you consider the comment I made in #25 even if it may mean switching format strings to PRId64 and alike? Or do you think it is too much / has too many side effects across a codebase with plenty of long etc?

@mbroz
Copy link
Contributor Author

mbroz commented Apr 30, 2024

Do you mean to use printf("%" PRId64", (int64_t)state->flen); or so ? It should do basically the same, just it needs more includes for type definitions...

@mbroz
Copy link
Contributor Author

mbroz commented Apr 30, 2024

(I tried to avoid change in exported structs. But if you want to change it, then to replace off_t with explicit 64bit type is definitely better. But despite it is binary equvalent, it guess it will be treated as API change by some tools...)

@eddelbuettel
Copy link
Owner

Hm. That is tricky either way.

@mbroz
Copy link
Contributor Author

mbroz commented Apr 30, 2024

Hm. That is tricky either way.

yes :) I think the whole code is full of old C. On the other side, off_t is really not a good choice here. Anyway, let me know what variant you prefer, I can change it once I get back to it.

@eddelbuettel
Copy link
Owner

@rabaucke Sorry for the delays. How do you feel about this one and possibly rebasing / simplifying #25? (Or replacing it if that is simpler.)

Not to trying to make everybody's life harder, just trying to make sure we don't break any delicate china in the shop....

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

Successfully merging this pull request may close these issues.

2 participants