boostorg / exception

Boost.org exception module
http://boost.org/libs/exception
Boost Software License 1.0
15 stars 46 forks source link

boost::exception should be nothrow-copy-constructible #15

Closed tonyelewis closed 5 years ago

tonyelewis commented 5 years ago

clang-tidy's cert-err60-cpp check has alerted me to CERT Coding Standard rule ERR60-CPP that exception types should be nothrow-copy-constructible.

At present, a boost::exception fails this clang-tidy check and std::is_nothrow_copy_constructible_v agrees…

#include <type_traits>

#include <boost/exception/all.hpp>

static_assert( ::std::is_nothrow_copy_constructible_v< ::boost::exception > );
> clang++ -std=c++17 -stdlib=libc++ -fsyntax-only -isystem include -isystem /opt/include a.cpp
a.cpp:5:1: error: static_assert failed due to requirement '::std::is_nothrow_copy_constructible_v< ::boost::exception>'
static_assert( ::std::is_nothrow_copy_constructible_v< ::boost::exception > );
^              ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 error generated.
> g++ -std=c++17 -fsyntax-only -isystem include -isystem /opt/include a.cpp 
a.cpp:5:16: error: static assertion failed
 static_assert( ::std::is_nothrow_copy_constructible_v< ::boost::exception > );

This comment in the code gives the impression that we might reasonably hope exception might be nothrow-copy-constructible and that it could be marked as such with some of Boost.Config's BOOST_NOEXCEPT macros.

Is this possible?

If a boost::exception is sometimes/always not nothrow-copy-constructible, it would be good to highlight this issue in the docs so that people are aware if/when their use of Boost.Exception violates a CERT rule.

Thanks for your work on this library.

zajo commented 5 years ago

boost::exception does not throw on copy. As to BOOST_NOEXCEPT, the reason it hasn't been used is that originally, <boost/exception/exception.hpp> was extremely light, it did not #include any headers, not even standard headers, nevermind <boost/config.hpp>.

I see that at some point we did end up including <boost/config.hpp>, so this should be an easy change.

By the way, <boost/exception/exception.hpp> isn't in this repo.

tonyelewis commented 5 years ago

Thanks for you reply. That all makes sense. Good news.

Apologies re the repos - the lack of issues on the throw_exception repo made me think it might be treated as a subsidiary of this one. GitHub has a new "Transfer this issue" feature in Beta so you could try transferring this across. Or I'm happy to reopen there and reference this?

Thanks.

zajo commented 5 years ago

No that's fine, let's leave it here. Logically, <boost/exception/exception.hpp> is part of Boost Exception. I'll wait until the current Boost release cycle is over to make this change. Or you can send a pull request.

zajo commented 5 years ago

bfddc104c6a5701d508a52e62eaed225670e6910