Is it defined behaviour to assign to function call in or in if (C 17)-CodePudding

During a codebase refactor I found code like this:

void myFunction (std::map<int, int> my_map)
{
    int linked_element;
    if (my_map[linked_element = firstIndex] != 0
        || my_map[linked_element = secondIndex] != 0)
    {
        // do some stuff with linked_element
    }
}

void myFunction (std::set<int> my_set)
{
    int linked_element;
    if (my_set.find(linked_element = firstIndex) != my_set.end()
        || my_set.find(linked_element = secondIndex) != my_set.end())
    {
        // do some stuff with linked_element
    }
}

From what I understood the aim of that was to avoid checking 2 times (first when entering in the if, second when assigning the variable). I can understand that depending on which side of the || is true linked_element will be assigned to the right value but this still feels kind of bad to me.

Is this kind of behaviour defined?

CodePudding user response：

This behavior is well defined by the order of evaluation.

First, the linked_element = firstIndex assignment happens. This expression returns the value of firstIndex, that is then used as an argument for the subscript operator on my_map (i.e., my_map[linked_element = firstIndex]). The return value from that expression is checked against the != 0 condition. If it's true, the other side of the || operator is not evaluated due to short-circuit logic. If it's false, the same story happens on the other side of the operator.

Whether or not it's a good practice to write code in such a style is a different question though. Personally speaking, I'd prioritize readability and maintainability over this micro-optimization unless it's a super-critical piece of the program, but it's a matter of opinion, I guess.

CodePudding user response：

In original code behavior is well defined, since operator || evaluates first argument and if this is evaluated to false evaluates second argument.

BUT: Assignment there is confusing and many (probably all) static analyzes tools will complain about this. So I would reflector this code in this way, so it would require less brain power to read:

void doSomeStuff(const std::set<int>& my_set, int linked_element)
{
    .....
}

void myFunction (const std::set<int>& my_set)
{
    if (my_set.find(firstIndex) != my_set.end())
    {
        doSomeStuff(my_set, firstIndex);
    } else if (my_set.find(secondIndex) != my_set.end()) {
        doSomeStuff(my_set, secondIndex);
    }
}

Since you had to ask question about this code this proves that original version is bad from maintainer point of view. Code which requires lots of focus to understand is costly in maintenance.

BTW this fragment of code:

if (my_map[linked_element = firstIndex] != 0

looks suspicious. I have even more suspensions seeing set-version. This looks like that someone do not understand how operator[] works for maps. If value for key do not exist, default value is introduced to map. So checking for default value 0 seem like attempt to adders this issue. Possibly my_map.count(firstIndex) should be used.

CodePudding user response：

An alternate version, assuming firstIndex and secondIndex are literal values (like 2 and 7), or are otherwise known relative to some invalid third index value:

void myFunction (std::set<int> & my_set)
{
    int linked_element =
        my_set.contains (firstIndex)  ? firstIndex  :
        my_set.contains (secondIndex) ? secondIndex :
        thirdIndex;
    if (linked_element != thirdIndex)
    {
        // do some stuff with linked_element
    }
}

If the indices are not known then a std::optional<int> can step in here too.

If pre-C 20, replace .contains() with .count().

Bigger concerns with the original code are:

the pass-by-value of a potentially large container (never assume COW)
map[index] silently adds the index to the map if not present