Skip to content

Fixed msvc and clang warnings #794

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

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

Conversation

TheMostDiligent
Copy link

No description provided.

sum = prog_a + prog_b + prog_c + prog_d;
sprintf(buffer, "%lu", sum);
sum = (unsigned long)(prog_a + prog_b + prog_c + prog_d);
snprintf(buffer, sizeof(buffer), "%lu", sum);

Choose a reason for hiding this comment

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

Is it targeted for demos to be also ANSI C89 (ISO C90) compatible?

Copy link
Author

Choose a reason for hiding this comment

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

snprintf is C99. Is there any compiler an existence that does not support C99 though?

There is a definite issue here and in other similar places: %lu defines unsigned long argument of size 4. sizeof(size_t) is 8 on 64- bit systems, so there is an argument size mismatch.

Choose a reason for hiding this comment

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

Apparently the reviewer's guide clarifies that for demos newer language features are not discouraged. But the library shall remain fully ANSI C89 compliant.

C99 introduced the "%zu" format specifier for size_t. I recommend removing the unsigned long casts and using zu.

Copy link

@klei1984 klei1984 Mar 28, 2025

Choose a reason for hiding this comment

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

This file seems to be the script generated output file that should not be changed by hand. The /src/*.* files are the source code of the library.

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