Fix compiler warnings #9411

Al2Klimov · 2022-06-22T17:34:15Z

No description provided.

Al2Klimov · 2022-07-25T15:59:02Z

@julianbrost Would you prefer me finish this or have this merged "ASAP" not to have it laying around for too long?

Al2Klimov · 2022-07-25T16:41:32Z

Btw. any idea what's the matter with the unused OpenSSL functions?

Al2Klimov · 2022-07-25T17:09:08Z

TODO

Have a look at Windows

julianbrost · 2022-07-26T07:31:49Z

@julianbrost Would you prefer me finish this or have this merged "ASAP" not to have it laying around for too long?

What are the thing you would or wouldn't do in each case?

But I'd rather keep this PR at a reviewable size rather than growing it to some monster nobody got time for to review.

Btw. any idea what's the matter with the unused OpenSSL functions?

Which ones do you mean?

Al2Klimov · 2022-07-26T09:00:23Z

@julianbrost Would you prefer me finish this or have this merged "ASAP" not to have it laying around for too long?

What are the thing you would or wouldn't do in each case?

But I'd rather keep this PR at a reviewable size rather than growing it to some monster nobody got time for to review.

I'm done anyway with not Windows-specific stuff.

Btw. any idea what's the matter with the unused OpenSSL functions?

Which ones do you mean?

lib/base/tlsutility.cpp:33:13: warning: unused function 'OpenSSLLockingCallback' [-Wunused-function]
lib/base/tlsutility.cpp:41:22: warning: unused function 'OpenSSLIDCallback' [-Wunused-function]

julianbrost · 2022-07-26T09:14:25Z

Btw. any idea what's the matter with the unused OpenSSL functions?

Which ones do you mean?
lib/base/tlsutility.cpp:33:13: warning: unused function 'OpenSSLLockingCallback' [-Wunused-function]
lib/base/tlsutility.cpp:41:22: warning: unused function 'OpenSSLIDCallback' [-Wunused-function]

They are used with some functions up-to-date OpenSSL versions no longer use. So figure out if it's still used in any OpenSSL version we have to support and if so, put it in #ifdef, otherwise remove it.

Al2Klimov · 2022-07-26T09:24:36Z

Oh, that's the point! They are inside #ifdefs... but reported as unused?!

julianbrost · 2022-07-26T09:28:42Z

They are inside #ifdef CRYPTO_LOCK, not some version check. So probably some "do it if that version of OpenSSL already supports it", now it's no longer used and some defines were left there so that they don't actively break code.

Al2Klimov · 2022-07-26T09:32:34Z

To me they seem to be still used. (Just search for them via Ctrl+F in the same file.) And the usages are in identical ifdefs.

julianbrost · 2022-07-26T09:37:10Z

Used with this macro that expands to nothing (i.e. no use of the function):

#  define CRYPTO_set_locking_callback(func)

Which is inside:

# if OPENSSL_API_COMPAT < 0x10100000L

So looks like that function was deprecated with 1.1.0, and I think CentOS 7 still uses an older version (:

Al2Klimov · 2022-07-26T10:08:51Z

Cool. /s

julianbrost

In general, if we take the effort to address compiler warnings, we should also do something to ensure that it stays this way, i.e. CI.

I see two options here:

Add a GH Actions job that compiles with -Werror. But this has the risk that at some point a new type of warning is added, not immediately addressed and the check will just be ignored.
I don't know if something like this already exists, but I'd be surprised if it doesn't: GH Actions allow to annotate the code in a PR, so it should be possible to annotate it with compiler warnings.

Can you please look if something like this exists and integrate it?

julianbrost · 2022-07-28T14:44:29Z

CMakeLists.txt

+add_definitions(-DBOOST_ALLOW_DEPRECATED_HEADERS)
+


What's the reason for this change? Basically that just silences a warning, why can't what the warning is actually about be addressed?

We have to support a wide range of Boost versions, with possibly different headers being available/deprecated or not. We already (have to) support [1.66, 1.79], 14 different versions. If we gonna support e.g. CentOS 7 till 2024, that will increase. I consider it more productive not to care about header deprecation.

The purpose of that warning should be a heads-up notice that if you address it, you won't be surprised by the next Boost version breaking something.

Now I haven't looked at this in detail (would actually addressing these warnings involve adding huge amounts of #ifdef?) and how Boost handles deprecation (do deprecated headers stay there basically forever?).

do deprecated headers stay there basically forever?

Unfortunately I forgot my crystal ball. So I opt for hoping for that, but changing this strategy on bad surprise.

I mean: just have a look at the add_definitions immediately nearby.

julianbrost · 2022-07-28T14:48:29Z

lib/base/configuration.cpp

-void Configuration::SetApiBindHost(const String& val, bool suppress_events, const Value& cookie)
+void Configuration::SetApiBindHost(const String& val, bool, const Value&)


I'm not a fan of addressing unused parameters like this. The names also serve a bit of a descriptive purpose. Without them, you have to find the signature that's implemented here or the call site to figure out what these parameters actually are.

Thus, I'd prefer (void) suppress_events; in instances like this. For something like MessageOrigin messageOrigin I'm fine with removing the name.

For used params, sure. One has to know what's what. But for unused? Who cares what that random bool is if no one uses it? After all in the (rare) case of (rare) cases these two red/green arrows in CLion next to the funcdef are your friends. Btw. we already do it like this.

julianbrost · 2022-07-28T15:02:25Z

lib/base/dependencygraph.cpp

-		typedef std::pair<Object *, int> kv_pair;
-		for (const kv_pair& kv : it->second) {
+		typedef std::pair<Object *const, int> kv_pair;
+		for (kv_pair& kv : it->second) {


Just use auto and get rid of the typedef?

lib/base/scriptutils.cpp

Al2Klimov · 2022-07-28T16:56:08Z

Actually this PR has never been about compiler warnings cleanup by itself. I as a dev just don’t want to get spammed with them as that hides everything else. I consider the current amount of warnings just fine. Apropos! Do you remember my TODO list? Its only point is checkboxed because I actually took a look at Windows (compiler warnings). Not more and not less. I decided not to care (for now), primarily to get this done. So no reason to overengineer anything.

Al2Klimov · 2023-03-23T11:48:13Z

Or to say it with your wording:

But I'd rather keep this PR at a reviewable size rather than growing it to some monster nobody got time for to review.

Al2Klimov · 2023-03-23T12:25:21Z

By the way. Depending on the bison/flex version one can get these warnings:

build/tools/mkclass/class_lexer.cc:2090:58: warning: unused parameter 'yyscanner' [-Wunused-parameter]
static void yy_fatal_error (yyconst char* msg , yyscan_t yyscanner)
                                                         ^
build/tools/mkclass/class_lexer.cc:2434:43: warning: unused parameter 'yyscanner' [-Wunused-parameter]
void *yyalloc (yy_size_t  size , yyscan_t yyscanner)
                                          ^
build/tools/mkclass/class_lexer.cc:2439:58: warning: unused parameter 'yyscanner' [-Wunused-parameter]
void *yyrealloc  (void * ptr, yy_size_t  size , yyscan_t yyscanner)
                                                         ^
build/tools/mkclass/class_lexer.cc:2451:36: warning: unused parameter 'yyscanner' [-Wunused-parameter]
void yyfree (void * ptr , yyscan_t yyscanner)

@yhabteab You as a Mac user can fix these easily:

brew install bison
brew install flex
cmake: add -DBISON_EXECUTABLE=/usr/local/Cellar/bison/3.8.2/bin/bison -DFLEX_EXECUTABLE=/usr/local/Cellar/flex/2.6.4_2/bin/flex

Al2Klimov · 2023-03-24T12:34:06Z

Used with this macro that expands to nothing (i.e. no use of the function):
#  define CRYPTO_set_locking_callback(func)
Which is inside:
# if OPENSSL_API_COMPAT < 0x10100000L
So looks like that function was deprecated with 1.1.0, and I think CentOS 7 still uses an older version (:

Yes, 1.0.2k. Good to know. openssl/openssl#1260 (comment)

…ariables

Al2Klimov self-assigned this Jun 22, 2022

cla-bot bot added the cla/signed label Jun 22, 2022

Al2Klimov force-pushed the compiler-warnings branch from 2cccd7c to 2dad63e Compare June 23, 2022 12:25

Al2Klimov changed the title ~~Silence compiler warnings~~ Fix compiler warnings Jun 23, 2022

Al2Klimov force-pushed the compiler-warnings branch 5 times, most recently from 83d5d60 to 0a416bd Compare June 30, 2022 16:53

Al2Klimov added this to the 2.14.0 milestone Jul 25, 2022

Al2Klimov force-pushed the compiler-warnings branch from 0a416bd to 9da3001 Compare July 25, 2022 17:09

Al2Klimov removed their assignment Jul 26, 2022

Al2Klimov marked this pull request as ready for review July 26, 2022 15:40

julianbrost reviewed Jul 28, 2022

View reviewed changes

julianbrost mentioned this pull request Aug 2, 2022

GHA: test -DICINGA2_UNITY_BUILD=ON|OFF #9469

Open

Al2Klimov force-pushed the compiler-warnings branch from 9da3001 to a0f1cab Compare August 16, 2022 09:43

Al2Klimov requested a review from julianbrost August 16, 2022 09:43

julianbrost removed this from the 2.14.0 milestone Jan 17, 2023

Al2Klimov mentioned this pull request Jan 19, 2023

Namespace: use rwlock and disable read locking after freeze #9627

Merged

1 task

Al2Klimov added this to awaits concept review (i.e. TBD) in 42 quadrillions PRs Jan 31, 2023

Al2Klimov force-pushed the compiler-warnings branch from a0f1cab to 15b00e3 Compare March 23, 2023 11:48

Al2Klimov force-pushed the compiler-warnings branch 2 times, most recently from 3cf4202 to f68520d Compare March 23, 2023 12:40

Al2Klimov self-assigned this Mar 24, 2023

Al2Klimov force-pushed the compiler-warnings branch from f68520d to ee0c5c4 Compare March 24, 2023 16:05

Al2Klimov removed their assignment Mar 24, 2023

Al2Klimov force-pushed the compiler-warnings branch 2 times, most recently from a5adc0f to 3ecc62d Compare March 24, 2023 17:08

Al2Klimov removed the request for review from julianbrost March 24, 2023 17:21

Al2Klimov self-assigned this Mar 24, 2023

Al2Klimov force-pushed the compiler-warnings branch 2 times, most recently from 3e1988e to abfeba7 Compare March 31, 2023 10:42

Al2Klimov mentioned this pull request Apr 3, 2023

GHA: Linux: forbid compiler warnings #9733

Draft

Al2Klimov added 7 commits April 3, 2023 11:06

Fix compiler warnings by not unnecessarily (copy-)constructing loop v…

0692dcb

…ariables

Fix compiler warnings by re-ordering member init in constructors

7c16672

Fix compiler warnings by not comparing signed with unsigned

9558e88

Fix compiler warnings by replacing x&&y||z with (x&&y)||z

ec54d64

Fix compiler warnings by ensuring variable initialization

09a7eb6

Fix compiler warnings by not std::move()ing where redundant

970d89e

Fix compiler warnings

12bf837

Al2Klimov force-pushed the compiler-warnings branch from abfeba7 to 12bf837 Compare April 3, 2023 09:06

Al2Klimov mentioned this pull request Apr 11, 2023

Silence compiler warnings about unused parameters #9728

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix compiler warnings #9411

Fix compiler warnings #9411

Al2Klimov commented Jun 22, 2022

Al2Klimov commented Jul 25, 2022

Al2Klimov commented Jul 25, 2022

Al2Klimov commented Jul 25, 2022 •

edited

Loading

julianbrost commented Jul 26, 2022

Al2Klimov commented Jul 26, 2022

julianbrost commented Jul 26, 2022

Al2Klimov commented Jul 26, 2022

julianbrost commented Jul 26, 2022

Al2Klimov commented Jul 26, 2022

julianbrost commented Jul 26, 2022

Al2Klimov commented Jul 26, 2022

julianbrost left a comment

julianbrost Jul 28, 2022

Al2Klimov Jul 28, 2022

julianbrost Jul 29, 2022

Al2Klimov Jul 29, 2022

Al2Klimov Aug 16, 2022

julianbrost Jul 28, 2022

Al2Klimov Jul 28, 2022

julianbrost Jul 28, 2022

Al2Klimov commented Jul 28, 2022

Al2Klimov commented Mar 23, 2023

Al2Klimov commented Mar 23, 2023

Al2Klimov commented Mar 24, 2023

		void Configuration::SetApiBindHost(const String& val, bool suppress_events, const Value& cookie)
		void Configuration::SetApiBindHost(const String& val, bool, const Value&)

		add_definitions(-DBOOST_ALLOW_DEPRECATED_HEADERS)

Fix compiler warnings #9411

Are you sure you want to change the base?

Fix compiler warnings #9411

Conversation

Al2Klimov commented Jun 22, 2022

Al2Klimov commented Jul 25, 2022

Al2Klimov commented Jul 25, 2022

Al2Klimov commented Jul 25, 2022 • edited Loading

TODO

julianbrost commented Jul 26, 2022

Al2Klimov commented Jul 26, 2022

julianbrost commented Jul 26, 2022

Al2Klimov commented Jul 26, 2022

julianbrost commented Jul 26, 2022

Al2Klimov commented Jul 26, 2022

julianbrost commented Jul 26, 2022

Al2Klimov commented Jul 26, 2022

julianbrost left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Al2Klimov commented Jul 28, 2022

Al2Klimov commented Mar 23, 2023

Al2Klimov commented Mar 23, 2023

Al2Klimov commented Mar 24, 2023

Al2Klimov commented Jul 25, 2022 •

edited

Loading