search

cartographer-project / cartographer

Cartographer is a system that provides real-time simultaneous localization and mapping (SLAM) in 2D and 3D across multiple platforms and sensor configurations.
Apache License 2.0
7.09k stars 2.25k forks source link

Add missing virtual destructors, fix #1771 #1773

Closed Tobias-Fischer closed 3 years ago

Tobias-Fischer commented 3 years ago

Signed-off-by: Tobias Fischer info@tobiasfischer.info

See #1771

Successfully fixed the test failures on osx on conda-forge & compiler does not complain anymore (previous warning example: warning: delete called on 'cartographer::mapping::Grid2D' that is abstract but has non-virtual destructor [-Wdelete-abstract-non-virtual-dtor].

johuber commented 3 years ago

I'm curious to see if with the requested changes this compiler warning will be rsolve.. because everything was already done correctly before, the only thing this changes is more explicit syntax.

johuber commented 3 years ago

I do not understand this PR. To me it looks like the compiler error is a false positive since both GridInterface and RangeDataInserterInterface already define a virtual destructor. Can you explain what you think is wrong with the code?

If this is indeed to mitigate a compiler issue: did you file an issue about this compiler issue?

I fully agree! marking them override cannot harm though and maybe suffices to get rid of the compiler warning

Tobias-Fischer commented 3 years ago

Hi there,

Thank you for your comments!

I do not have any knowledge of cartographer - this PR originated (as described above, where I also link to the issue https://github.com/cartographer-project/cartographer/issues/1771 which contains the compiler warnings and more information) from failing test cases on OSX when packaging cartographer for conda-forge.

After applying the changes in this PR, both the compiler warnings (not "compiler error", I nowhere talk of compiler errors), as well as the test failures, disappear. So I am not sure how you think that "everything was done correctly before" and "the compiler error is a false positive". Things obviously weren't all done correctly before, nor was it a false positive, as there were test failures (albeit only on OSX with clang, but nonetheless) that are resolved after applying this PR.

You are more than welcome to make the requested changes in this PR if you want to improve cartographer by fixing these test failures, edits from the maintainers are allowed. If you still think this PR is only addressing false positives, I am equally happy to close this PR, and continue to use a patch to fix the problems purely in conda-forge.

wolfv commented 3 years ago

Hey @johuber @wohe

here are the build & test logs for cartographer on osx-64, without the fixes applied: https://dev.azure.com/conda-forge/feedstock-builds/_build/results?buildId=235112&view=logs&jobId=58ac6aab-c4bc-5de2-2894-98e408cc8ec9&j=58ac6aab-c4bc-5de2-2894-98e408cc8ec9&t=933f325c-924e-533d-4d95-e93b5843ce8b

With the fixes applied all builds are green: https://github.com/conda-forge/cartographer-feedstock/pull/7

We now provide up-to-date cartographer builds on conda-forge for Windows, OS X and Linux :)

Btw, if either of you is interested in helping to maintain cartographer on conda-forge, please let me know and we can add you as maintainer. This usually doesn't involve a lot of effort, just now and then updating the version if a new release appears (there are bots for this, too).

wolfv commented 3 years ago

Hm, I think the issue is that the 1.0.0 which is quite old by now didn't have the virtual destructor in the base class. E.g. this PR https://github.com/cartographer-project/cartographer/pull/1453 hasn't made it in the 1.0.0 release as far as I can see. How about a new release? Looks like there is quite a bit of development since 1.0 :)

So I guess we could also fix our build in conda-forge by adding the empty virtual destructor in the GridInterface base class. Either way, what we have works for the moment.

Tobias-Fischer commented 3 years ago

Thanks for investigating this @wolfv! Seems like this issue does not occur on the master branch anymore - apologies for the confusion caused. Closing here