C++ suggest parentheses around assignment used as truth value

Question

I have some problems with this part of my code:

Employees.cpp:

void Employees::delete_employee()
{
    int employee_number;

    cout<<"\n..."<<endl<<endl;
    cout<<"Give me the number:";
    cin>>Employee_number;

    for(std::vector<EmployeeStruct>::size_type i = 0; i != lista.size(); i++)
        {
            if (employee_number = lista[i].Employee_number)
            {
                lista.erase(lista.begin() + i);
            }
        }
    cout<<"..."<<endl<<endl;
    cout<<"--------------------------------"<<endl;

}

When I compile my code I see something like that: warning: suggest parentheses around assignment used as truth value [-Wparentheses] and I really don't know what to do with that. Maybe it isn't a serious problem, but my program doesn't work like I want. (this code should delete employee)

In main.cpp I have:

    case 4:
        {
            Pracownicy p;
            p.usun_pracownika();
        }
    break;

Maybe somebody can help me.

You want `employee_number == lista[i].Employee_number`. Notice ***assignment used as truth value***. — LogicStuff, Feb 01 '16 at 19:29
Use [Erase-remove idiom](https://en.wikipedia.org/wiki/Erase%E2%80%93remove_idiom) to fix your other error. — Jarod42, Feb 01 '16 at 19:30
... or *don't* increment `i` after you've just erased an element. — LogicStuff, Feb 01 '16 at 19:31

score 2 · Accepted Answer · edited May 23 '17 at 12:04

2

I'm realy don't know what to do with that.

It depends on what you intended to do.

If you didn't intend to assign, but rather, compare equality, then you should fix the code to use the equality operator: ==

If you did intend to assign, then you can: a) Ignore the warning or b) add the parentheses which tells the compiler that you really intend to do an assignment and that it was not a mistake. Or as commented by Christian Hackl c) Split assignment and comparison into two statements for increased readability.

There is also another bug in your code. The erase shifts the rest of the elements left. The next element is no longer at i++, so you end up skipping elements after an erase. This is also inefficient if more than one element is erased. See erase-remove idiom for a better way.

edited May 23 '17 at 12:04

Community

1
1

answered Feb 01 '16 at 19:30

eerorika

232,697
12
197
326

c) Split assignment and comparison into two statements for increased readability. – Christian Hackl Feb 01 '16 at 19:31
It's clear from the code that comparison was intended. I think instead of " erase invalidates the erase" you *meant* "the erase invalidates iterators and references after the erased item", which, after think about it, you would realize doesn't matter, but that possibly skipping over the end matters. Apart from skipping over the end (bug) it' just an inefficient, quadratic time, way of removing items. – Cheers and hth. - Alf Feb 01 '16 at 20:01
Yes, but there's no iterator here. It's plain indexing. ;-) – Cheers and hth. - Alf Feb 01 '16 at 20:18
@Cheersandhth.-Alf wow, totally true. I must have glanced over the code too fast. Removed iterator stuff from the answer now. – eerorika Feb 01 '16 at 20:22
@Cheersandhth.-Alf It might be clear that comparison was intended, but someone might stumble upon this question about the warning having actually done the assignment on purpose, so that's to make my answer more general. – eerorika Feb 01 '16 at 20:30

score 0 · Answer 2 · answered Feb 01 '16 at 19:30

I think it is this very common error:

if (employee_number == lista[i].Employee_number)
                    ^^
{
     lista.erase(lista.begin() + i);
}

You have confused the boolean check == operator for the = assignment operator here, either as a typo or by mistake.

score 0 · Answer 3 · answered Feb 01 '16 at 19:30

I'm realy don't know what to do with that.

Use a equals comparison instead of the assignment, that's what the warning finally suggests in most cases:

 if (employee_number == lista[i].Employee_number)
                  // ^^

or make it clear what you want using parenthesis around

if ((employee_number = lista[i].Employee_number))

Not of such a good warning of the compiler though.

score 0 · Answer 4 · answered Feb 01 '16 at 19:32

0

A simple mistake. You wrote:

 if (employee_number = lista[i].Employee_number)

write

if (employee_number == lista[i].Employee_number)

answered Feb 01 '16 at 19:32

deepakgupta8777

26
4

score 0 · Answer 5 · answered Feb 01 '16 at 19:52

The main problem on your side is inadvertent use of the assignment operator = instead of the intended comparison ==.

On the language's side this common problem was enabled via two risk-enhancing features inherited from original C, namely that a formal condition accepts any object with a conversion to bool, and that, in support of multi-assignments, assignment is an expression rather than a statement.

You were lucky to compile with a compiler and options that produced a warning about it: such warnings are not guaranteed.

A guaranteed way to instead get an error is to let the left hand side be an expression that can't be assigned to, e.g.

use const liberally, wherever it makes sense.

In the given case, instead of

void Employees::delete_employee()
{
    int employee_number;

    cout<<"\n..."<<endl<<endl;
    cout<<"Give me the number:";
    cin>>Employee_number;

    for(std::vector<EmployeeStruct>::size_type i = 0; i != lista.size(); i++)
        {
            if (employee_number = lista[i].Employee_number)
            {
                lista.erase(lista.begin() + i);
            }
        }
    cout<<"..."<<endl<<endl;
    cout<<"--------------------------------"<<endl;

}

you can do the safer

auto fail() -> bool ...;

auto line_from( istream& stream )
    -> string
{
    string line;
    getline( stream, line )
        or fail();
    return line;
}

void Employees::delete_employee()
{
    cout<<"\n..."<<endl<<endl;
    cout<<"Give me the number:";
    int const Employee_number = to_string( line_from( cin ) );

    for(std::vector<EmployeeStruct>::size_type i = 0; i != lista.size(); i++)
        {
            if (employee_number == lista[i].Employee_number)
            {
                lista.erase(lista.begin() + i);
            }
        }
    cout<<"..."<<endl<<endl;
    cout<<"--------------------------------"<<endl;

}

This code will not compile at all if == is used instead of =, and as a bonus using getline helps to avoid many common problems with input of values from cin.

A different kind of fix is to redefine if via a macro, or to define a macro that expands to a safe use of if. The first is technically possible but formally Undefined Behavior if any standard library header is included, which is usually the case. The second is a severe (usually unacceptable) uglification of the code, and will make maintenance a nightmare. As I recall something like that was once done for one of the Unix shells. Since nobody but the original programmer understood any of the code, it had a short life.

C++ suggest parentheses around assignment used as truth value

5 Answers5

Linked