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

Jump to bottom

Fix warnings #3095

Merged
merged 2 commits into from
Sep 13, 2022
Merged

Fix warnings #3095

merged 2 commits into from
Sep 13, 2022

Conversation

HazardyKnusperkeks
Copy link
Contributor

Fix some warnings.

vitaut
vitaut requested changes Sep 12, 2022
View reviewed changes
Copy link
Contributor

@vitaut vitaut left a comment

Choose a reason for hiding this comment

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

Thank for the PR. In general looks good except for the -Wsuggest-attribute=format part which is a bit messy. Considering that this code is going away I suggest not trying to suppress it.

@phprus
Copy link
Contributor

phprus commented Sep 12, 2022

@vitaut
Tell me please, sprintf is only used to print floating point numbers in hex format? Or not?

@vitaut
Copy link
Contributor

vitaut commented Sep 12, 2022

Tell me please, sprintf is only used to print floating point numbers in hex format?

Yes, s(n)printf is only used for hexfloat.

@HazardyKnusperkeks
Suppress -Wfloat-equal
7cbb045 
Only NaN and Inf are not less than Inf and the check for NaN is done
before.

Solves:
.../fmt/include/fmt/format.h:2509:43: warning: comparing floating-point with '==' or '!=' is unsafe [-Wfloat-equal]
 2509 |     return !detail::isnan(value) && value != inf && value != -inf;
@HazardyKnusperkeks
Suppress -Wshadow
a2f9dea 
Solves:
/fmt/include/fmt/ostream.h:89:18: warning: declaration of 'fbuf' shadows a previous local [-Wshadow]
   89 |   else if (auto* fbuf = dynamic_cast<__gnu_cxx::stdio_filebuf<char>*>(rdbuf))
      |                  ^~~~
C:/GIT/ok-mimot/libs/3rdParty/fmt/include/fmt/ostream.h:87:13: note: shadowed declaration is here
   87 |   if (auto* fbuf = dynamic_cast<__gnu_cxx::stdio_sync_filebuf<char>*>(rdbuf))
      |             ^~~~
@HazardyKnusperkeks
Copy link
Contributor Author

Thank for the PR. In general looks good except for the -Wsuggest-attribute=format part which is a bit messy. Considering that this code is going away I suggest not trying to suppress it.

Is there an ETA on when it's going away?

@vitaut vitaut merged commit 2d66ad5 into fmtlib:master Sep 13, 2022
@vitaut
Copy link
Contributor

vitaut commented Sep 13, 2022

Merged, thanks!

Is there an ETA on when it's going away?

it will happen in the next (major) release some time next year. The actual code might be removed earlier in preparation for the release.

@HazardyKnusperkeks HazardyKnusperkeks deleted the fix-warnings branch September 14, 2022 07:55
@HazardyKnusperkeks
Copy link
Contributor Author

it will happen in the next (major) release some time next year. The actual code might be removed earlier in preparation for the release.

We are hitting this warning in our tests (-Werror). When the code is going away, couldn't we just add the suppression, as it also will go away? I'd rather not change the our submodule to a fork.

@vitaut
Copy link
Contributor

vitaut commented Sep 14, 2022

You can suppress the warnings with FMT_SYSTEM or other method. I don't think it makes sense to add a suppression that won't be included in any release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vitaut vitaut vitaut approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@HazardyKnusperkeks @phprus @vitaut