tags:

views:

41

answers:

2

I ran across the error Socket operation on non-socket in some of my networking code when calling connect and spent a lot of time trying to figure out what was causing it. I finally figured out that the following line of code was causing the problem:

if ((sockfd = socket( ai->ai_family, ai->ai_socktype, ai->ai_protocol) < 0)) {

See the problem? Here's what the line should look like:

if ((sockfd = socket( ai->ai_family, ai->ai_socktype, ai->ai_protocol)) < 0) {

What I don't understand is why the first, incorrect line doesn't produce a warning. To put it another way, shouldn't the general form:

if ( foo = bar() < baz ) do_something();

look odd to the compiler, especially running with g++ -Wall -Wextra?

If not, shouldn't it at least show up as "bad style" to cppcheck, which I'm also running as part of my compile?

+5  A: 

Actually, you don't get any warning because of the double parenthesis (.

Try to remove one pair, and you'll get the warning back.

#include <iostream>

int foo()
{
    return 2;
}

int main(int /*argc*/, char** /*argv*/)
{
    int l;

    if ((l = foo() < 3)) // Won't generate warning under gcc
    {
    }

    if (l = foo() < 3) // will generate a warning "warning: suggest parentheses around assignment used as truth value"
    {
    }

    return EXIT_SUCCESS;
}

To avoid such annoying mistakes/typos, I avoid assigning a value and testing it in the same statement. That's too much error prone imho.

ereOn
Why do the extra parens make a difference?
Robert S. Barnes
@Robert S. Barnes: I guess this was initialy made to prevent mistakes like writing `if (i = 5)` instead of `if (i == 5)`. If you really want to do `if (i = 5)`, the `gcc` way to remove this warning is to surround the assignation with parenthesis, thus doubling it, as to indicate "this is not a typo, this is **meant** !". Probably not a perfect solution, as your question shows !
ereOn
I would think that cppcheck would catch both cases, but surprisingly it doesn't even catch the second case without the extra `()`.
Robert S. Barnes
+2  A: 

That's one reason why I try not to do too much in one statement. Instead of

if ((sockfd = socket( ai->ai_family, ai->ai_socktype, ai->ai_protocol)) < 0) {

Why not:

sockfd = socket( ai->ai_family, ai->ai_socktype, ai->ai_protocol)
if(sockfd < 0) {
zildjohn01
Exactly. It's bad style to put too much in one statement. Also it's also much easier to debug if you put the if on the next line.
humbagumba
While I might agree with the general statement, "Don't do too much in one statement," I don't know that it applies here. There is practical value to conciseness in code, and this particular C/C++ idiom has been around for a long time and is basically standard practice. I've been programming for about 10 years and IIRC this is the first time I've run into this problem.
Robert S. Barnes
I would suggest that any practical value gained by conciseness (kinda curious as to what value you're thinking of) is countered by a drop in readability. If you have to pause on a line or do a double-take just to figure out exactly what effect it has, it's probably better to break it down.
JTeagle