Make parsing of values syntactically more strict with bad values generating an error#244
Make parsing of values syntactically more strict with bad values generating an error#244
Conversation
|
@osrf-jenkins run tests |
|
I think there are some test files missing that should start with |
|
|
||
| ss >> posetmp; | ||
| this->dataPtr->value = posetmp; | ||
| } |
There was a problem hiding this comment.
I think the changes to this block are not needed because Param::SetFromString already calls sdf::trim to remove whitespace and then sets a default value if the trimmed string is empty. I've added tests to confirm this in 9daad39 of branch scpeters/stricter_parsing_10 after reverting the changes to this block in 8f6120c
There was a problem hiding this comment.
Sounds good. I see that ValueFromString is also called form the constructor without sdf::trim, but that should be okay because it's using the default string from the spec description.
https://github.com/osrf/sdformat/blob/b80e070dbed613e972acfb81e66b24a248eefd6c/src/Param.cc#L70
There was a problem hiding this comment.
I just tried it and it looks like UNIT_Utils_TEST fails with that change.
There was a problem hiding this comment.
ok, it sounds like this change is a good idea. Perhaps we could generalize it so it applies to types other than Pose?
There was a problem hiding this comment.
I think we discussed this in our last meeting, and this was the last change we expected to make?
Oops! I've added them in 2b42034 |
|
there are test failures on macOS and windows; I've investigated the macOS failure, and it seems that the parsing the bad pose file doesn't generate an exception with the CI version of clang / libc++ |
Codecov Report
@@ Coverage Diff @@
## sdf10 #244 +/- ##
==========================================
+ Coverage 86.64% 86.66% +0.01%
==========================================
Files 59 59
Lines 9067 9071 +4
==========================================
+ Hits 7856 7861 +5
+ Misses 1211 1210 -1
Continue to review full report at Codecov.
|
|
do you want to target this at |
…rating an error Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>
Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>
Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>
Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>
libc++ doesn't throw an exception on failbit, so check the bit by calling `fail()` Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>
Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>
Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>
Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>
2fa77df to
e59c6d7
Compare
Done. |
…rating an error (#244) Currently, libsdformat silently ignores syntax errors when parsing values. For example, <pose>bad 0 0 0 0 0</pose> or <pose>0.1 0.2 0.3 0.4 0.5</pose> are both bad values for the <pose> tag, but the parsed values are <pose>0 0 0 0 0 0</pose> and <pose>0.1 0.2 0.3 0.4 0.5 0</pose> respectively. With this PR, libsdformat will generate an error for such values.
Signed-off-by: Steve Peters <scpeters@openrobotics.org>
…#308) * Remove all deprecated *PoseFrame() APIs * Remove deprecated Pose(), SetPose() APIs * Migration guide: update per gazebosim#244, gazebosim#276 * Changelog for gazebosim#308. Signed-off-by: Steve Peters <scpeters@openrobotics.org>
…#308) * Remove all deprecated *PoseFrame() APIs * Remove deprecated Pose(), SetPose() APIs * Migration guide: update per gazebosim#244, gazebosim#276 * Changelog for gazebosim#308. Signed-off-by: Steve Peters <scpeters@openrobotics.org>
Currently, libsdformat silently ignores syntax errors when parsing values. For example,
<pose>bad 0 0 0 0 0</pose>or<pose>0.1 0.2 0.3 0.4 0.5</pose>are both bad values for the<pose>tag, but the parsed values are<pose>0 0 0 0 0 0</pose>and<pose>0.1 0.2 0.3 0.4 0.5 0</pose>respectively. With this PR, libsdformat will generate an error for such values.The plan is to make a similar PR for libsdformat9, but printing a warning message instead.
This resolves #228