libgearman/client.cc compilation errors

[jwakely]

libgearman/client.cc: In function 'gearman_return_t gearman_client_add_server(gearman_client_st*, const char*, in_port_t)':
libgearman/client.cc:602:69: error: ISO C++ forbids comparison between pointer and integer [-fpermissive]
     if (gearman_connection_create(client->universal, host, port) == false)
                                                                     ^~~~~
libgearman/client.cc: In member function 'gearman_return_t Client::add_server(const char*, const char*)':
libgearman/client.cc:617:63: error: ISO C++ forbids comparison between pointer and integer [-fpermissive]
   if (gearman_connection_create(universal, host, service_) == false)
                                                               ^~~~~
libgearman/client.cc: In function 'gearman_return_t gearman_client_job_status(gearman_client_st*, const char*, bool*, bool*, uint32_t*, uint32_t*)':
libgearman/client.cc:949:21: error: ISO C++ forbids comparison between pointer and integer [-fpermissive]
     if (is_known == false and is_running == false)
                     ^~~~~
libgearman/client.cc:949:45: error: ISO C++ forbids comparison between pointer and integer [-fpermissive]
     if (is_known == false and is_running == false)
                                             ^~~~~

Somebody is confused about the difference between pointers and booleans.

The first two are easy to fix (see #75) but the other one isn't so obvious. Is it supposed to be checking for null pointers?

 if (is_known == NULL and is_running == NULL)

or this, would would crash if they're null (and the checks a few lines above suggest they can be):

 if (*is_known == false and *is_running == false)

or maybe:

 if ((!is_known or *is_known == false) and (!is_running or *is_running == false))

or something else entirely.

Please advise, and I'll update the pull request.

[p-alik]

Thank you for reporting, @jwakely.

[p-alik]

Some TODO's with regard to the issue:

tests/protocol.cc:  ASSERT_TRUE(connection1= gearman_connection_create(universal, GEARMAN_DEFAULT_TCP_HOST, libtest::default_port()));
tests/protocol.cc:  ASSERT_TRUE(connection1= gearman_connection_create(universal, GEARMAN_DEFAULT_TCP_HOST, libtest::default_port()));
tests/protocol.cc:  ASSERT_TRUE(connection1= gearman_connection_create(universal, GEARMAN_DEFAULT_TCP_HOST, libtest::default_port()));
tests/libgearman-1.0/regression.cc:    ASSERT_TRUE(con_ptr= gearman_connection_create(universal, NULL, default_port()));
tests/libgearman-1.0/regression.cc:    ASSERT_TRUE(con_ptr= gearman_connection_create(universal, NULL, default_port()));
tests/libgearman-1.0/worker_test.cc:  ASSERT_TRUE(connection1= gearman_connection_create(universal, NULL, default_port()));
tests/libgearman-1.0/worker_test.cc:  ASSERT_TRUE(connection2= gearman_connection_create(universal, NULL, default_port()));

[SpamapS]

I'd like to get this one in and release 1.1.15 soon as there are other unreleased fixes in master. I don't have a lot of time to look at the issue, but I'd say one approach is to ensure there's adequate test coverage here, and make the most obvious change.

[esabol]

I'm only a novice at reading the gearmand code, but, after looking it over, I think it should be:

   if (is_known == NULL and is_running == NULL)

[p-alik]

@esabol, I'll test it twice to be on the safe side.

[esabol]

Do those ASSERT_TRUE lines mentioned above result in compilation errors or warnings? If so, I think they could be fixed, like so:

ASSERT_TRUE(whatever) => ASSERT_TRUE((whatever) != NULL)

What do you think?

[p-alik]

actually none of them.
I guess ASSERT_TRUE should be replaced with a check for nullptr because gearman_connection_create is of type gearman_connection_st *.