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 compiler warnings on EL6,EL7,EL8 #76

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

fix compiler warnings on EL6,EL7,EL8 #76

wants to merge 1 commit into from

Conversation

pbiering
Copy link
Contributor

split-out of #71

@@ -42,7 +42,7 @@ class AndingFilter : public Filter
Filter *stealLastSubfiler();
bool accepts(void *data);
void combineFilters(int count, int andor);
unsigned numFilters() { return _subfilters.size(); }
int numFilters() { return _subfilters.size(); } // satisfy -Wsign-compare
Copy link
Contributor

Choose a reason for hiding this comment

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

But we cannot have a negative number of filters. I think we want unsigned here. On what line does it complain about the comparison?

@@ -52,15 +52,15 @@ extern char *log_file;

int num_cached_log_messages = 0;

LogCache::LogCache(unsigned long max_cached_messages)
LogCache::LogCache(long max_cached_messages) // satisfy -Wsign-compare
Copy link
Contributor

Choose a reason for hiding this comment

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

Unsigned seems correct to me, should look at the comparison instead

: _max_cached_messages(max_cached_messages)
, _num_at_last_check(0)
{
pthread_mutex_init(&_lock, 0);
updateLogfileIndex();
}

void LogCache::setMaxCachedMessages(unsigned long m)
void LogCache::setMaxCachedMessages(long m) // satisfy -Wsign-compare
Copy link
Contributor

Choose a reason for hiding this comment

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

Unsigned seems correct to me, should look at the comparison instead

@@ -173,7 +173,7 @@ void LogCache::dumpLogfiles()
it != _logfiles.end();
++it)
{
Logfile *log = it->second;
// Logfile *log = it->second; // satisfy -Wunused-variable
Copy link
Contributor

Choose a reason for hiding this comment

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

Line can be deleted

@@ -49,7 +49,7 @@ double ServiceSpecialDoubleColumn::getValue(void *data)
if (is_cmk_passive) {
host *host = svc->host_ptr;
servicesmember *svc_member = host->services;
double check_interval = 1;
// double check_interval = 1; // satisfy -Wunused-variable
Copy link
Contributor

Choose a reason for hiding this comment

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

Can be deleted

@@ -51,7 +51,7 @@ int32_t ServicelistStateColumn::getValue(int logictype, servicesmember *mem, Que
{
contact *auth_user = query->authUser();
int32_t result = 0;
int lt;
// int lt; // satisfy -Wunused-variable
Copy link
Contributor

Choose a reason for hiding this comment

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

Can be deleted

@@ -378,7 +378,7 @@ static gboolean by_one_group(gpointer _name, gpointer _hst, gpointer user_data)

void TableHosts::answerQuery(Query *query)
{
struct hostbygroup **_hg_tmp_storage = (struct hostbygroup **)&(query->table_tmp_storage);
// struct hostbygroup **_hg_tmp_storage = (struct hostbygroup **)&(query->table_tmp_storage); // satisfy -Wunused-variable
Copy link
Contributor

Choose a reason for hiding this comment

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

Delete line

Copy link
Contributor

@jacobbaungard jacobbaungard left a comment

Choose a reason for hiding this comment

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

Overall looks good, but could you have a look at all the changes from unsigned to signed. I think all cases it makes sense to have unsigned, as negative numbers makes no sense in that context.

@pbiering
Copy link
Contributor Author

Overall looks good, but could you have a look at all the changes from unsigned to signed. I think all cases it makes sense to have unsigned, as negative numbers makes no sense in that context.

in underlying resulting assembler code, usually comparison of integers are done by substracting them (and therefore converting them to signed integer before) and check whether result is greater/equal/lower than zero. Looks like compilers get more strict here to prevent any accidents by substracting integers which have value >= 2^(bitlength-1), because then the bit for the +/- get lost.

You can avoid the compiler warning by casting during comparison, but this is also not a nice way and can cause issues for same reason.

@jacobbaungard
Copy link
Contributor

What I am saying is: We should use consistent types throughout the source, and it makes no sense to change an value which is intrinsically non-negative to a signed integer, just because somewhere we are doing a comparison with a signed integer. Instead we should ensure the other variable we are comparing with is also unsigned.

For example rather than changing _max_cached_messages to signed, we should change num_cached_log_messages to unsigned.

@pbiering
Copy link
Contributor Author

What I am saying is: We should use consistent types throughout the source, and it makes no sense to change an value which is intrinsically non-negative to a signed integer, just because somewhere we are doing a comparison with a signed integer. Instead we should ensure the other variable we are comparing with is also unsigned.

For example rather than changing _max_cached_messages to signed, we should change num_cached_log_messages to unsigned.

feel free to fix the other side...I have only tried to fix where compiler warned me -> cherry pick the other valid changes and adjust the others...

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