Re: [boost] [Stacktrace] review

Subject: Re: [boost] [Stacktrace] review
From: Nat Goodspeed (nat_at_[hidden])
Date: 2016-12-14 14:42:14

On Wed, Dec 14, 2016 at 3:43 AM, Niall Douglas <s_sourceforge_at_[hidden]>
wrote:

For your review you may wish to consider the following questions:
>
>       - What is your evaluation of the design?
>

I'm generally pleased with the design.

There's a documentation section "Exceptions with stacktrace" that discusses
techniques for incorporating stacktrace data into your own exceptions. I
see a real possibility for integration with Boost.Exception, perhaps even a
mechanism to make BOOST_THROW_EXCEPTION embed the stacktrace at the throw
point. This is not an objection to accepting the library, but rather a
feature request for when we do.

I do have a question about use of a template parameter to constrain the
number of entries, rather than a runtime parameter. Presumably that's
necessary to permit certain backend implementations to be
async-signal-safe? (Whatever the rationale, a note in the documentation
explaining it would be nice.)

My objection to that is that apparently, with the present design, if I want
to write a function that accepts any specialization of basic_stacktrace,
that must itself be a template function.

If I'm correct about the reason for using a template parameter, would it be
reasonable to split the class into a non-template base class supporting all
the methods, with a template subclass that provides the storage? The base
class could store a (pointer, length) passed by the subclass constructor.
Without having looked at the implementation, I expect that you wouldn't
even need virtual-function dispatch: the only indirection would be through
the data buffer pointer.

That would even permit a subclass that provides dynamic storage, for
async-signal-unsafe use.

Then a processing function could accept reference-to-base-class and handle
any stacktrace instance.

>       - What is your evaluation of the implementation?

Sorry, I haven't looked at the implementation.

>       - What is your evaluation of the documentation?
>

Generally good. It could stand a cleanup pass; there are a few typos here
and there.

I do like that each method explicitly states both complexity and async
signal safety.

The description of the nullary frame() constructor says that source_line()
will return both empty string and 0.

>       - What is your evaluation of the potential usefulness of the
>         library?
>

VERY! I want this.

>       - Did you try to use the library?  With what compiler?  Did you
>         have any problems?
>

No, I haven't tried to use it. (A deadline is coming up fast.)

>       - How much effort did you put into your evaluation? A glance? A
>         quick reading? In-depth study?
>

I read through the documentation.

>       - Are you knowledgeable about the problem domain?
>

I have occasionally used platform-specific tactics to try to achieve the
same thing, on a couple different platforms. Each time it's been painful
enough to make me wish that this problem were solved, once and for all.

> 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. I would like to see the non-template base class, but that's not a
showstopper: I would rather have this library without than not have it at
all.

As I mentioned, I would like to see integration with Boost.Exception. That
just seems like a natural next step.

I see further opportunities for integration with Boost.Context,
Boost.Fiber...