boostorg / exception

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

enable_error_info concerns #50

Closed zlojvavan closed 1 year ago

zlojvavan commented 1 year ago

hello I wonder what's the point of copying exception object "If T derives from boost::[exception]" instead of returning it (to throw later) as is?

zajo commented 1 year ago

This is a very old library from before we had move. What is the concern with the copy? Presumably we're using this just before we throw, so any concerns about speed are probably not warranted. Also, if you pass an unnamed temporary to enable_error_info, the copy constructor is elided.

zlojvavan commented 1 year ago

just the fact that there's unnecessary copy operation that probably could be avoided even without move semantics if that's what you meant

well, yes I used it before uniformly but figured it does unnecessary copy so I've had to separate my throw helper in two distinct ones

template <typename T> [[noreturn]] std::enable_if_t< std::is_base_of_v<boost::exception, T>> ThrowExceptionImpl(T const& be, char const* current_function, char const* file, int line) { ProcessErrorInfoContainer(be); ... } template <typename T> [[noreturn]] std::enable_if_t< !std::is_base_of_v<boost::exception, T> > ThrowExceptionImpl(T const& e, char const* current_function, char const* file, int line) { ThrowExceptionImpl(boost::enable_error_info(e), current_function, file, line); }

thought you would be interested to optimize it away without relying upon copy elision

zajo commented 1 year ago

What is the concern, the the size of the generated code or the speed?

zlojvavan commented 1 year ago
  1. the fact there's unnecessary copy operation is enough for me
  2. I'm not so sure about "if you pass an unnamed temporary to enable_error_info, the copy constructor is elided", see https://godbolt.org/z/vvdj9ooY3

according to Explanation "1) First, copy-initializes the exception object from expression" and though there's also "The copy/move (since C++11) may be subject to copy elision" part and my example might be wrong IIRC in my experience I observed copy operation

p.s. despite being "old" I find that lib very useful and wish all boost libs used boost::exception/throw_exception and boost/std offered customization points for throwing exceptions (as already asked in https://github.com/boostorg/exception/issues/25#issuecomment-587312449) as some environments such as codegear/embarcadero allow

zajo commented 1 year ago

I also checked whether the copy elision happens, but it doesn't because it turns out std::~exception is defined out of line. Actually if you use boost::throw_exception there is no problem right? It's got its own mechanism for injecting boost::exception as a base.

zlojvavan commented 1 year ago

so apparently I was right that copy elision on throw is wishful thinking and boost::throw_exception(derived_exception()) still invokes copy constructor and increments the counter

zajo commented 1 year ago

Anyway, I don't see a practical problem with any of this.

Have you considered using Boost LEAF instead? It is a better solution to the same problem, assuming its limitations are not an issue for your use case.

zlojvavan commented 1 year ago

well of course it's up to you whether to fix it or not, though probably cost of copying exception in some derived hierarchies might be not so low. at least I think I did my best to highlight the problem

I believe I considered LEAF couple of years ago and while I find it really interesting and will probably start using it in new projects some day I guess it cannot replace current infrastructure (including boost/exception) in existing projects/libs

zajo commented 1 year ago

I prefer to not make unnecessary changes to Boost Exception, as I can not test with old MSVC versions.

Here is guide on how to migrate from Boost Exception to LEAF, it's straight forward.

Also, you can handle Boost Exception error info with LEAF.

zlojvavan commented 1 year ago

even if it's "straight forward" atm there's no motivation and sometimes technical feasibility to refactor all existing sources to migrate to LEAF