Skip to content

Fix UnitTestArrayPortalValueReference

A recent change to the continuous integration setup has caused UnitTestArrayPortalValueReference to fail on one platform.

The problem was that the test was doing two unsafe things with the right-shift assignment operator. The first unsafe thing was using itself as the operand.

  ref >>= ref;

This causes clang to give a "self assign overload" warning. Using a variable as its own operand for a compute assign operation isn't great style, but for some operations it can cause weird errors. The reason for the warning is that for a true integer shift operation, the compiler will recognize that the result should be 0. So, when using this on base integer types, you will get 0. But overloads can give you something different, so that might lead to unexpected results. Because we are using an overload (for the ArrayPortalValueReference class), the compiler tells us we can get potentially unexpected results.

OK. That seems like something we can safely ignore (and were ignoring for some time). After all, the whole point of the ArrayPortalValueReference operators is to behave exactly the same as the values they wrap. That brings us to the second unsafe thing the test was doing: using an invalid value as the right hand operation. And this is where things get weird.

The test was specifically failing when being run on Int32. It was setting the underlying value for ref to be 1000 to start with. So the expression ref >>= ref was trying to right shift ref by 1000 bits. Logically, this should of course give you 0. However, shifting a number by more bits than exist causes undefined behavior (c.f. https://wiki.sei.cmu.edu/confluence/display/c/INT34-C.+Do+not+shift+an+expression+by+a+negative+number+of+bits+or+by+greater+than+or+equal+to+the+number+of+bits+that+exist+in+the+operand). You might not get back the expected value, and this is exactly what was happening.

What I think happened was that the compiler determined that any valid shift would be contained in 5 bits, so it truncated the value (1000) to the least signifcant 5 bits (which happens to be 8). It then shifted 1000 by 8 and got the value 3 instead of 0.

The fix picks an operand number that is sure to be valid.

Merge request reports

Loading