Review guidelines =================
Reviews should be submitted to the developer list (boost@lists.boost.org), preferably with '[stacktrace]' in the subject. Or if you don't wish to for some reason or are not subscribed to the developer list you can send them privately to me at 's_sourceforge at nedprod dot com'. If so, please let me know whether or not you'd like your review to be forwarded to the list.
For your review you may wish to consider the following questions:
- What is your evaluation of the design? Seems like the right approach to me. You have the stacktrace that stores
the trace and you can get more information through the frame class. Since the discussion about attaching a stack-trace to an exception got rolling, I'd like to propose two functions: boost::stacktrace::throw_with_st<std::runtime_error>("bla"); //throws an exception derived from std::runtime_error and ... catch(std::runtime_error & re) { boost::stacktrace::rethrow_with_st(re); //rethrows, but has the stacktrace attached }; I don't think the latter would work with std::exception_ptr, but if it does, that would be even better.
- What is your evaluation of the implementation? Most of my personal concerns with this library are with the implementation and I would hugely appreciate feedback from others with substantial experience of using stacktracing "in anger" in non-trivial use case scenarios.
Seems to be the overall right way to me. However there are a few problems: - backend_linux/add2line Do I have a guarantee that this tool works right for every compiler on linux? I would instinctively think not. If I'm right here, there should at the very least be a macro which allows to change the program name. Additionally I would require that <windows.h> is not included in backend_windows.hpp, but to go the route of boost/winapi or to put this include in the source. Or to rename backend_windows.hpp to backend_windows.ipp, so the intent is obvious. Also, I get why there's the stack-trace function at the top of the stacktrace, and it would be the wrong approach to remove it manually. I would however like a workaround, so that the stacktrace doesn't include those calls. Something like that maybe: (CaptureStackBackTrace is obtained through a fwd-declaration as in boost/winapi) #define BOOST_STACKTRACE(Name) \ boost::stacktrace::stacktrace Name(boost::stacktrace::detail::empty_tag()); \ BOOST_FILL_STACKTRACE(Name); #if defined(BOOST_STACKTRACE_WINDOWS) #define BOOST_FILL_STACKTRACE(Obj) \ { \ boost::detail::winapi::ULONG_ hc = 0; \ ::CaptureStackBackTrace(0,static_cast<boost::detail::winapi::ULONG_>( boost::stacktrace::stacktrace::size), \ Obj.native_handle(), &hc); \ } #endif The downside would be, that this wouldn't work well with the link-solution for noop. But maybe instead of using CaptureStackBackTrace directly, this could be done through a function-ptr given by the linked library, so that it can point to a noop. It would also be awesome if you could provide a better build description, especially since this library may be used by other boost libraries in the future. Something like that: lib foo : bar.cpp : <stacktrace>off ; //not link to anything lib foo : bar.cpp : <stacktrace>noop ; //link against the empty backend lib foo : bar.cpp : <stacktrace>windbg ; //windows ... lib foo : bar.cpp : <stacktrace>on ; //automatically select the default with debug, none with release. I think if that is defined in stacktrace/build/Jamfile.v2 it would only be available when building boost or having boost through use-project. That would be very helpful.
- What is your evaluation of the documentation?
I think that about right.
- What is your evaluation of the potential usefulness of the library? - Did you try to use the library? With what compiler? Did you have any problems?
Yes, mingw 6 (didn't work) and msvc 19, where it worked. I got an error with msvc (RtlCaptureStackBackTrace not defined), because I hadn't defined _WIN32_WINNT correclty. It might be useful to add an error/warning for that.
- How much effort did you put into your evaluation? A glance? A quick reading? In-depth study?
Tried a simple example, executed the tests, looked through the implementation. ~2h.
- Are you knowledgeable about the problem domain?
No, i.e. not more than knowing what a stacktrace is.
And finally, every review should attempt to answer this question:
- Do you think the library should be accepted as a Boost library?
Yes. I do think it can be improved by further additions, but the basis is sound.