Niall Douglas wrote:
3. Please could people actually review Stacktrace's implementation code instead of inferring implementation from what someone else said here about something they half read in the tutorial. Specifically, is its API design solid? Is the use of std::string in its public API acceptable?
There's nothing wrong with using std::string in principle, but it's obviously not async safe. The operations however that return std::strings are also not async safe at the moment, so it's not the std::string return value that's the problem. The documentation has an example of printing a stack trace from a SIGSEGV handler, which is undefined behavior as it stands. So it's not clear to me whether the library is supposed to be useful - in its decoding/printing part, not just in its capture part - in signal handlers. Changing/augmenting the interface to be async safe would be easy, size_t frame::get_name( char* buffer, size_t size ) const; instead of, or in addition to, std::string frame::name() const; and void frame::print_to_fd( int fd ) const; in addition to ostream& operator<<( ostream&, frame const & ); but if the implementation can't be made async safe, this is not going to be of much use. A quick look of the implementation - and the list of POSIX async safe functions - was not enough for me to be able to resolve this question one way or the other. Most of what the Linux backend is doing is already async safe, but not everything, addr2line has an option to demangle, so the dependency on core::demangle could in principle be avoided... but I'm not quite sure. There is also an indication that the current frame interface is not the right one. In std::string name = f.name(); if (!name.empty()) { os << name; } else { os << f.address(); } const std::size_t source_line = f.source_line(); if (source_line) { os << " at " << f.source_file() << ':' << source_line; } which is perfectly reasonable code given the current interface, addr2line is called three times instead of just one. And in operator<< for a stacktrace of depth N, addr2line will be called 3*N times instead of one. The operator<< implementations themselves can be fixed, but user code that does the same will obviously not be able to benefit. I understand why it's better for the value_type of stacktrace::const_iterator to be 'frame' instead of just 'void*', but the accessors each calling addr2line is a symptom of a serious problem that the 'stupid' void* design: void get_frame_info( void* address, string& function, string& file, size_t& line ); lacks. stacktrace itself could provide a function that calls a user-provided callback void process_frame_info( size_t index, void* address, char const* function, char const* file, size_t line ); for each frame. This would be able to call addr2line just once, and it's also an async-safe interface, if the implementation can be made so. This is not exactly a common C++ style, but async safe code obviously has its own rules.