Properly handle stream errors when reading math objects

[roehling]

Many classes use temporary variables in operator>> to read data from the
stream. However, if stream operations fail, the variables will not be
modified, and the temporary variables, which are POD data types, remain
uninitialized. Often, but not always, this happens to be zero, which is
probably why this bug remained undetected for so long.

This is ultimately the reason why the sdformat Utils_TEST sometimes
fails with an empty pose string: the empty string causes the
uninitialized values to propagate to the Pose, while the unit test
clearly expects the default value.

This PR modifies the >> operators such that an invalid input will not
change the underlying object at all.

[codecov]

Codecov Report

Merging #180 (fa8d692) into ign-math6 (44a17a4) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           ign-math6     #180   +/-   ##
==========================================
  Coverage      99.19%   99.19%           
==========================================
  Files             61       61           
  Lines           5978     5981    +3     
==========================================
+ Hits            5930     5933    +3     
  Misses            48       48           

Impacted Files	Coverage Δ
include/ignition/math/Quaternion.hh	100.00% <ø> (ø)
include/ignition/math/Vector2.hh	100.00% <ø> (ø)
include/ignition/math/Vector3.hh	95.75% <ø> (ø)
include/ignition/math/Vector4.hh	94.44% <ø> (ø)
include/ignition/math/Color.hh	100.00% <100.00%> (ø)
include/ignition/math/Matrix3.hh	100.00% <100.00%> (ø)
include/ignition/math/Matrix4.hh	100.00% <100.00%> (ø)
include/ignition/math/Temperature.hh	100.00% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 44a17a4...fa8d692. Read the comment docs.

[traversaro]

Great catch @roehling !

I may be wrong, but I guess this is a problem that would have been detected by running the tests suite under Valgrind or the MemorySanitizer. In my experience the systematic use of Valgring is easier (even if slower during the tests), as it does not require to rebuild the library but only to add few lines of CMake in the macro that adds tests, see for example https://github.com/dic-iit/bipedal-locomotion-framework/blob/e43cc1827fa1e2062f117fc37f1bf986cf6b5fc3/cmake/AddBipedalLocomotionUnitTest.cmake#L19 and https://github.com/dic-iit/bipedal-locomotion-framework/blob/e43cc1827fa1e2062f117fc37f1bf986cf6b5fc3/cmake/AddBipedalLocomotionUnitTest.cmake#L76 .

[roehling]

Valgrind is really good (I actually used it to discover that the underlying problem was an uninitialized value), but tends to eat lots of RAM for its bookkeeping.

[traversaro]

Valgrind is really good (I actually used it to discover that the underlying problem was an uninitialized value), but tends to eat lots of RAM for its bookkeeping.

That is exactly the reason why in https://github.com/dic-iit/bipedal-locomotion-framework/blob/e43cc1827fa1e2062f117fc37f1bf986cf6b5fc3/cmake/AddBipedalLocomotionUnitTest.cmake#L19 we have a separate option to enable the (additional) Valgrind tests. Normal developers just pass FRAMEWORK_COMPILE_tests=ON to get the usual non-Valgrind tests suite, then in CI (or if a developer wants to debug those) also FRAMEWORK_RUN_Valgrind_tests=ON is passed, that runs every test in the test suite also under Valgrind. In this way the work of the developers is not slowed down, until the CI catches a Memory-related problem in a test.

[scpeters]

thanks for the fix!

[scpeters]

it looks like this fixes gazebosim/sdformat#407