Remove egregious error of removing from a set being range iterated over #9796
Conversation
Addresses issue 9791
There was a problem hiding this comment.
+@RussTedrake for feature review (and feel free to steal the patch in the short-term if you have to.)
Reviewable status:
complete! all discussions resolved, Exempt from platform review
There was a problem hiding this comment.
Feature 
Reviewed 1 of 1 files at r1.
Reviewable status: all discussions resolved, LGTM missing from assignee russtedrake, Exempt from platform review
There was a problem hiding this comment.
Thanks @sherm1. The more interesting question here is the conspicuous absence of a test. I'd love to reproduce the failure condition @RussTedrake had and commit a test with this fix to bolster up its claim to being a fix.
Reviewable status: all discussions resolved, LGTM missing from assignee russtedrake, Exempt from platform review
|
Now it fails at |
There was a problem hiding this comment.
Nominally addressed offline.
Reviewable status: all discussions resolved, LGTM missing from assignee russtedrake, Exempt from platform review
|
fwiw -- we managed to "figure out" a way to make the world_id consistently 1 for both python and c++, but there was still come confusion on why the apple object was being treated incorrectly (even if world_id == 2). |
|
the "fix" we figured out this afternoon is here: |
|
The fix is an improvement, at least. GSG prohibits non-POD globals, for good reason. (The fix should use never_destroyed, though.) If you are having problems with globals in python vs C++ though, I wonder if you're hitting the ODR problems? Ah, yes -- depending on //examples/manipulation_station from a pydrake module as in https://github.com/RussTedrake/drake/blob/f62df45/bindings/pydrake/examples/BUILD.bazel#L66 is abusing the linker, and will give you multiple independent copies of your globals. You should add the manipulation_station to the install/libdrake list of bazel packages to link into libdrake.so, not deps on it directly. |
|
FrameId is a POD (just an int64_t) so shouldn't require never_destroyed. |
|
Its API contract does not promise that nobody will ever add a dtor (that is always trivially destructible). You could imagine someone adding |
There was a problem hiding this comment.
@RussTedrake given this is a patch to a dev branch (and it definitely addresses a defect) we should merge this. @sherm1 provided an LGTM so you can either unassign yourself or review this change. I leave it up to you.
Reviewable status: all discussions resolved, LGTM missing from assignee russtedrake, Exempt from platform review
There was a problem hiding this comment.
Reviewable status: all discussions resolved, LGTM missing from assignee russtedrake, Exempt from platform review
Addresses issue #9791
This change is