Clean up functions that trigger GCC9 warnings

[mjcarroll]

Removes unnecessary copy constructors and insures that all destructors are marked virtual where appropriate.

Signed-off-by: Michael Carroll michael@openrobotics.org

[codecov]

Codecov Report

Merging #261 (e159361) into main (2bab469) will decrease coverage by 0.07%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #261      +/-   ##
==========================================
- Coverage   82.22%   82.15%   -0.08%     
==========================================
  Files         109      111       +2     
  Lines        4558     4539      -19     
==========================================
- Hits         3748     3729      -19     
  Misses        810      810              

Impacted Files	Coverage Δ
tpe/lib/src/AABBTree.cc	91.04% <ø> (-0.26%)	⬇️
tpe/lib/src/CollisionDetector.cc	98.01% <ø> (-0.04%)	⬇️
tpe/lib/src/Entity.hh	0.00% <ø> (ø)
tpe/lib/src/Shape.cc	78.35% <ø> (-3.39%)	⬇️
tpe/lib/src/Link.hh	100.00% <100.00%> (ø)
tpe/lib/src/Shape.hh	100.00% <100.00%> (ø)
tpe/lib/src/AABBTree.hh	100.00% <0.00%> (ø)
tpe/lib/src/CollisionDetector.hh	100.00% <0.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 2bab469...e159361. Read the comment docs.

[chapulina]

The LinkFeatures test failure seems to be introduced by this PR?

[mjcarroll]

@osrf-jenkins retest this please

[scpeters]

I think the test failure will be resolved by #275

[chapulina]

No new warnings or failures are introduced by this, let's get it in.

[scpeters]

No new warnings or failures are introduced by this, let's get it in.

I disagree; the LinkFeatures test had a pre-existing problem but it was passing, while this PR turns it into a failure.

https://build.osrfoundation.org/view/ign-fortress/job/ignition_physics-ci-main-bionic-amd64/31/testReport/junit/(root)/UNIT_LinkFeatures_TEST/test_ran/

I think we should either:

• disable the LinkFeatures test on main until Add Gravity Feature, fix LinkFeatures_TEST #275 is merged forward
• revert this until Add Gravity Feature, fix LinkFeatures_TEST #275 is merged forward

[chapulina]

the LinkFeatures test had a pre-existing problem but it was passing, while this PR turns it into a failure.

Humm I'm still not sure this PR is the one introducing the failure. LinkFeatures also fails with a segfault in #272, which didn't include this PR yet.

I can't reproduce the failure locally, but I suspect it's not because of this PR. Something may have changed upstream (on SDF?) and affected main.

[scpeters]

the LinkFeatures test had a pre-existing problem but it was passing, while this PR turns it into a failure.

Humm I'm still not sure this PR is the one introducing the failure. LinkFeatures also fails with a segfault in #272, which didn't include this PR yet.

I can't reproduce the failure locally, but I suspect it's not because of this PR. Something may have changed upstream (on SDF?) and affected main.

the most recent GitHub action for this PR merged into main passes on focal but fails on bionic:

• https://github.com/ignitionrobotics/ign-physics/actions/runs/1017007343

[scpeters]

the LinkFeatures test had a pre-existing problem but it was passing, while this PR turns it into a failure.

Humm I'm still not sure this PR is the one introducing the failure. LinkFeatures also fails with a segfault in #272, which didn't include this PR yet.
I can't reproduce the failure locally, but I suspect it's not because of this PR. Something may have changed upstream (on SDF?) and affected main.

the most recent GitHub action for this PR merged into main passes on focal but fails on bionic:

• https://github.com/ignitionrobotics/ign-physics/actions/runs/1017007343

I'm also having trouble reproducing it locally

[chapulina]

passes on focal but fails on bionic:

I can't reproduce on Bionic locally though

[azeey]

I was able to reproduce it locally inside docker with dependencies installed from nightlies.

[azeey]

Here's my hypothesis. The test has been somewhat broken because sdf::World::Load is called instead of sdf::Root::Load. A consequence of that is the error message:

[Err] [SDFFeatures.cc:511] The link of the parent frame [base_link] of joint [upper_joint] in model [double_pendulum_with_base] could not be resolved. The joint will not be constructed
[Err] [SDFFeatures.cc:517] Error Code 9: [:L530708400]: Msg: Frame has invalid pointer to FrameAttachedToGraph.

that indicates that the parent link of the joint could not be resolved, which is because a frame graph is not available when sdf::World::Load is called. But when printing the error message, it segfaults because of gazebosim/sdformat#590. The issue is fixed by gazebosim/sdformat#610, but that hasn't made it to main yet.

[scpeters]

Here's my hypothesis. The test has been somewhat broken because sdf::World::Load is called instead of sdf::Root::Load. A consequence of that is the error message:

[Err] [SDFFeatures.cc:511] The link of the parent frame [base_link] of joint [upper_joint] in model [double_pendulum_with_base] could not be resolved. The joint will not be constructed
[Err] [SDFFeatures.cc:517] Error Code 9: [:L530708400]: Msg: Frame has invalid pointer to FrameAttachedToGraph.

that indicates that the parent link of the joint could not be resolved, which is because a frame graph is not available when sdf::World::Load is called. But when printing the error message, it segfaults because of ignitionrobotics/sdformat#590. The issue is fixed by ignitionrobotics/sdformat#610, but that hasn't made it to main yet.

gazebosim/sdformat#610 was merged forward to main in gazebosim/sdformat#613 on June 30th

[azeey]

ignitionrobotics/sdformat#610 was merged forward to main in ignitionrobotics/sdformat#613 on June 30th

Ah, you're right. So I dug a little deeper and I can now reproduce it with sdformat build from source. The trick is to build it with g++ 7.5 as it's done by the nightly debbuilder. My new hypothesis is that sdf::operator<<(std::ostream, const std::Error&) is inlined in ign-physics code because it's defined in sdf/Error.hh. And since ign-physics is compiled with g++ 8, there might be some weird ABI issues between symbols accessed by the inlined code and the ones available in libsdformat.so.

I see two options:

1. Move sdf::operator<<(std::ostream, const std::Error&) to Error.cc so it won't get inlined.
2. Use g++8 in the sdformat nightly debbuilder job.

I have tested locally that both would work.

[scpeters]

ignitionrobotics/sdformat#610 was merged forward to main in ignitionrobotics/sdformat#613 on June 30th

Ah, you're right. So I dug a little deeper and I can now reproduce it with sdformat build from source. The trick is to build it with g++ 7.5 as it's done by the nightly debbuilder. My new hypothesis is that sdf::operator<<(std::ostream, const std::Error&) is inlined in ign-physics code because it's defined in sdf/Error.hh. And since ign-physics is compiled with g++ 8, there might be some weird ABI issues between symbols accessed by the inlined code and the ones available in libsdformat.so.

I see two options:

1. Move sdf::operator<<(std::ostream, const std::Error&) to Error.cc so it won't get inlined.
2. Use g++8 in the sdformat nightly debbuilder job.

I have tested locally that both would work.

I prefer moving it to Error.cc

[scpeters]

ignitionrobotics/sdformat#610 was merged forward to main in ignitionrobotics/sdformat#613 on June 30th

Ah, you're right. So I dug a little deeper and I can now reproduce it with sdformat build from source. The trick is to build it with g++ 7.5 as it's done by the nightly debbuilder. My new hypothesis is that sdf::operator<<(std::ostream, const std::Error&) is inlined in ign-physics code because it's defined in sdf/Error.hh. And since ign-physics is compiled with g++ 8, there might be some weird ABI issues between symbols accessed by the inlined code and the ones available in libsdformat.so.
I see two options:

1. Move sdf::operator<<(std::ostream, const std::Error&) to Error.cc so it won't get inlined.
2. Use g++8 in the sdformat nightly debbuilder job.

I have tested locally that both would work.

I prefer moving it to Error.cc

trying it out in gazebosim/sdformat#623