Skip to content

Implement rule of zero#614

Merged
azeey merged 3 commits intogazebosim:mainfrom
azeey:rule_of_zero
Aug 6, 2024
Merged

Implement rule of zero#614
azeey merged 3 commits intogazebosim:mainfrom
azeey:rule_of_zero

Conversation

@azeey
Copy link
Copy Markdown
Contributor

@azeey azeey commented Jul 26, 2024

Closes #76

Summary

Most of it was deleting copy constructors, assignment operators and destructors. SignalStats needed a change to make it use gz::utils::ImplPtr.

Test it

Checklist

  • Signed all commits for DCO
  • Added tests
  • Added example and/or tutorial
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • Consider updating Python bindings (if the library has them)
  • codecheck passed (See contributing)
  • All tests passed (See test coverage)
  • While waiting for a review on your PR, please help review another open pull request to support the maintainers

Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining Signed-off-by messages.

Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>
@github-actions github-actions bot added the 🏛️ ionic Gazebo Ionic label Jul 26, 2024
@scpeters
Copy link
Copy Markdown
Member

I see one compiler warning

/Library/Developer/CommandLineTools/SDKs/MacOSX14.2.sdk/usr/include/c++/v1/__memory/unique_ptr.h:65:5: warning: delete called on 'gz::math::SignalStatistic' that is abstract but has non-virtual destructor [-Wdelete-abstract-non-virtual-dtor]
    delete __ptr;
    ^
/Library/Developer/CommandLineTools/SDKs/MacOSX14.2.sdk/usr/include/c++/v1/__memory/unique_ptr.h:297:7: note: in instantiation of member function 'std::default_delete<gz::math::SignalStatistic>::operator()' requested here
      __ptr_.second()(__tmp);
      ^
/Library/Developer/CommandLineTools/SDKs/MacOSX14.2.sdk/usr/include/c++/v1/__memory/unique_ptr.h:263:75: note: in instantiation of member function 'std::unique_ptr<gz::math::SignalStatistic>::reset' requested here
  _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_SINCE_CXX23 ~unique_ptr() { reset(); }
                                                                          ^
/usr/local/include/pybind11/pybind11.h:1974:40: note: in instantiation of member function 'std::unique_ptr<gz::math::SignalStatistic>::~unique_ptr' requested here
            v_h.holder<holder_type>().~holder_type();
                                       ^
/usr/local/include/pybind11/pybind11.h:1634:26: note: in instantiation of member function 'pybind11::class_<gz::math::SignalStatistic, gz::math::python::SignalStatisticTrampoline>::dealloc' requested here
        record.dealloc = dealloc;
                         ^
/Users/jenkins/jenkins-agent/workspace/gz_math-ci-pr_any-homebrew-amd64/gz-math/src/python_pybind11/src/SignalStats.cc:124:3: note: in instantiation of function template specialization 'pybind11::class_<gz::math::SignalStatistic, gz::math::python::SignalStatisticTrampoline>::class_<pybind11::buffer_protocol, pybind11::dynamic_attr>' requested here
  py::class_<Class, SignalStatisticTrampoline>(m,
  ^
1 warning generated.

@azeey azeey added the beta Targeting beta release of upcoming collection label Jul 29, 2024
Addisu Z. Taddese added 2 commits August 6, 2024 11:51
@azeey azeey merged commit 00deb01 into gazebosim:main Aug 6, 2024
@azeey azeey deleted the rule_of_zero branch August 6, 2024 18:28
@Crola1702
Copy link
Copy Markdown

@azeey I think this PR created a regression in gz_math8-install_bottle-homebrew-amd64

See log output:

In file included from test.cpp:1:
/usr/local/Cellar/gz-math8/7.999.999-0-20231006/include/gz/math8/gz/math/SignalStats.hh:25:10: fatal error: 'gz/utils/ImplPtr.hh' file not found
#include <gz/utils/ImplPtr.hh>
         ^~~~~~~~~~~~~~~~~~~~~
1 warning and 1 error generated.
Error: osrf/simulation/gz-math8: failed

Maybe we're missing a release from gz-utils in homebrew?

@azeey
Copy link
Copy Markdown
Contributor Author

azeey commented Aug 12, 2024

It looks like there's a test in the homebrew formula that builds this with manual compiler flags
https://github.com/osrf/homebrew-simulation//blob/64970778c6b13cc22739a47dc7e5b67aa491b5cc/Formula/gz-math8.rb#L58. I'll update it to add gz-utils.

@scpeters do we actually need this test? I would have thought the CMake test would be sufficient.

@azeey
Copy link
Copy Markdown
Contributor Author

azeey commented Aug 12, 2024

@Crola1702, "fix" in osrf/homebrew-simulation#2723

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

beta Targeting beta release of upcoming collection 🏛️ ionic Gazebo Ionic

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

4 participants