Simulation.h: fix type-matching and const errors in templates. #2008
Conversation
rpoyner-tri
commented
Apr 5, 2016
rpoyner-tri
commented
- simulate(5-arg): don't modify the passed options struct.
- restore const to options arg, to get sim(4) a fighting chance.
- simulate(4-arg): fix a typo that defeats template resolution.
|
@jwnimmer-tri: please review this PR. |
| if (options.realtime_factor < 0.0) options.realtime_factor = 0.0; | ||
| const SimulationOptions& options) { | ||
| SimulationOptions my_options = options; | ||
| if (my_options.realtime_factor < 0.0) my_options.realtime_factor = 0.0; |
There was a problem hiding this comment.
Did you investigate whether any calling code relies on this broken output-writeback behavior?
There was a problem hiding this comment.
FWIW, I did the grep for this, and didn't see any code that touched realtime_factor after simulate() or runLCM() is called. (I did find that runLCM re-implements the "negative means 1.0" rule, but shrug.)
|
Comments in notes. LGTM. |
|
@jwnimmer-tri: i did not investigate reliance on brokenness further than "no tests failed". @david-german-tri assures us us (from f2f discussion) that broader reform is imminent, so I didn't dig any deeper. As for sim(arity-4), it was clearly just dead code before I resuscitated it. |
|
I'll incorporate @david-german-tri 's suggestion and send it through again. |
* simulate(5-arg): (re?) const-ify options struct argument. * since that is clearly what simulate(4-arg) requires. * simulate(4-arg): fix a typo that defeats template resolution.
rpoyner-tri
force-pushed
the
simulation-fixes
branch
from
878315a to
b406d84
Compare
|
revised patch is up. @jwnimmer-tri , @david-german-tri , please have a look. Thanks! |
|
LGTM. |
|
LGTM as well |
|
@david-german-tri Hmm. I think we are going to need to close (and tag and) and reopen this, to get drake005 coverage? |
|
didn't drake0005 already run? |
|
Huh. When I commented, it had not, but its there now. Ah, I see. More of us got added to the whitelist, so that the "approved for build servers" tag is not required. |
|
Two reviews done, four CI reports green, bombs away! |
