Re: [boost] [Stacktrace] Second review begins today 17th Mar ends 26th Mar

Subject: Re: [boost] [Stacktrace] Second review begins today 17th Mar ends 26th Mar
From: Antony Polukhin (antoshkka_at_[hidden])
Date: 2017-03-18 10:26:54

2017-03-17 19:15 GMT+03:00 Peter Dimov via Boost <boost_at_[hidden]>:
>> You can find the documentation at
>> http://apolukhin.github.io/stacktrace/index.html and the github repo at
>> https://github.com/apolukhin/stacktrace.
>
>
> A few quick comments.
>
> 1. The library should try to not #include <windows.h> unless necessary.
> Constructing a stacktrace object and most of its operations besides
> operator<< / to_string do not need <windows.h> and it should be possible to
> avoid it. The functionality that needs <windows.h> needs to be defined in a
> separate header, for example <boost/stacktrace/output.hpp>.
>
> This allows use of the library in header-only mode while still isolating
> <windows.h> use into its own source file.
>
> For example, consider throw_with_trace in the documentation; the throw
> points, which in a header-only library will be in headers, do not need
> <windows.h> and should not drag it in. Only the catch point, which resides
> safely in its own source file, does.

<windows.h> is a lesser evil. The huge problem is a COM header with a
lot of things in global namespace and a bunch of unportable vendor
specific extensions. One pain of death, I'm not going to rewrite all
the COM macro-spaghetti code (but I'm open to pull requests). The only
good solution to avoid global namespace polution - is not to use the
library in header only mode on MSVC.

> Incidentally, safe_dump_win.ipp includes <windows.h>, but does not need to.

Fixed in develop

> 2. There should exist a documented way to construct a stacktrace from an
> array of void const*. This is currently possible via from_dump, but it's not
> guaranteed to work; the format of the dump is technically an implementation
> detail of the library and may change.

Where that could be useful?

> 3. detail::try_demangle is unnecessary, as it does the same thing as
> core::demangle. :-)

Thanks! Fixed in develop.

-- 
Best regards,
Antony Polukhin