Skip to content

Conversation

@jschmidt-icinga
Copy link
Contributor

@jschmidt-icinga jschmidt-icinga commented Oct 17, 2025

Together with #9411, #9728, #9729 and #9730 this fixes most of the remaining compiler warnings, except the following:

  • A few -Wdangling-reference in scheduleddowntime-apply.cpp, which I'm pretty sure are false positives.
  • The OpenSSL 3.0 -Wdeprecated-declarations warnings.
  • The '-Wdeprecated' warnings about %pure-parser being deprecated in the .yy parser files. At first I wanted to include them here, but despite things seemingly building fine it seems there's some additional implications with replacing %pure-parser with %define api.pure and %error-verbose with %define parse.error verbose that I can't judge because I'm not a bison expert.
  • Maybe additional warnings on MSVC if more than the default are turned on (/Wall)

Copy link
Member

@Al2Klimov Al2Klimov left a comment

Choose a reason for hiding this comment

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

Note: This currently duplicates some of the efforts in #9411 until that is merged.

Suggestion:

  1. Rebase #10609 on top of #9411
  2. Change your base branch from master to compiler-warnings

@Al2Klimov Al2Klimov self-requested a review October 20, 2025 08:28
@jschmidt-icinga jschmidt-icinga force-pushed the fix-misc-compiler-warnings branch 2 times, most recently from a25c3f1 to 289607b Compare October 21, 2025 13:44
@Al2Klimov Al2Klimov removed their request for review October 21, 2025 14:05
@yhabteab
Copy link
Member

  • The '-Wdeprecated' warnings about %pure-parser being deprecated in the .yy parser files. At first I wanted to include them here, but despite things seemingly building fine it seems there's some additional implications with replacing %pure-parser with %define api.pure and %error-verbose with %define parse.error verbose that I can't judge because I'm not a bison expert.

Oh, forgot to send my reply to this. These warning don't exist on my machine and replacing %pure-parser with %define api.pure {,true,full} results in a bison syntax error. I use the following bison version:

$ bison --version
bison (GNU Bison) 2.3
Written by Robert Corbett and Richard Stallman
---

And after going through the Bison docs, there's quite no difference between %pure-parser and %define api.pure true. There seems to be a difference between full and true using as a value though, but I guess that's irrelevant now, since we won't change this anytime soon.

@jschmidt-icinga jschmidt-icinga force-pushed the fix-misc-compiler-warnings branch from 289607b to 4389abf Compare November 17, 2025 15:57
@jschmidt-icinga jschmidt-icinga force-pushed the fix-misc-compiler-warnings branch from 4389abf to 179aa12 Compare November 19, 2025 10:04
@jschmidt-icinga
Copy link
Contributor Author

jschmidt-icinga commented Nov 19, 2025

  • After switching the compiler to clang it found a few new variations of unused variables which I fixed as well. Maybe we should add a runner that also tests with clang to get those warnings too.
  • Hopefully the windows warnings about branches not returning is now fixed by making the return type of icinga_assert_fail() function void. (Edit: Seems MSVC is still not happy)
  • I've changed the timestamps used for the replay log file names from using int to std::uint64_t. I didn't just leave them as double so they can easily be converted from and to strings from/for the file names.

This is necessary to stop MSVC complaining that "not all paths return a value", because
it is not able to infer that the expression `false ? 0 : non_returning_function()`
never returns.

In the process of debugging this I've also slightly simplified the other assert macros and
abort function, so they don't need compiler specific preprocessor-paths anymore.
@jschmidt-icinga jschmidt-icinga force-pushed the fix-misc-compiler-warnings branch from 179aa12 to 11e83b6 Compare November 19, 2025 11:01
@jschmidt-icinga
Copy link
Contributor Author

And the dumbest compiler award goes to... *drumroll* 🎉MSVC🎉, for the inability to infer that the expression (false ? 0 : noreturn_function()) does in fact not return. Something something industry standard.

Copy link
Member

@yhabteab yhabteab left a comment

Choose a reason for hiding this comment

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

LFTM! Ty.

@yhabteab yhabteab added this to the 2.16.0 milestone Nov 19, 2025
@yhabteab yhabteab added the core/quality Improve code, libraries, algorithms, inline docs label Nov 19, 2025
@yhabteab yhabteab merged commit ed90141 into master Nov 19, 2025
25 checks passed
@yhabteab yhabteab deleted the fix-misc-compiler-warnings branch November 19, 2025 13:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla/signed core/quality Improve code, libraries, algorithms, inline docs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants