2017-05-24 3:23 GMT+02:00 Niall Douglas via Boost :

On 23/05/2017 22:46, Andrzej Krzemienski via Boost wrote:

...

So, I am trying to run my first test program with Boost.Outcome. The library fails to compile with GCC 6.3.0 on MinGW on Windows 7. I have filed the issue: https://github.com/ned14/boost.outcome/issues/36

Mingw is not on the list of supported compilers at https://ned14.github.io/boost.outcome/prerequisites.html

I refuse to support Mingw, not worth the waste of my time. I will unofficially support Mingw-w64. Outcome had been tested on mingw-w64 some time ago, but it had become minor broken since, it's now fixed on develop branch. Thanks for the bug report.

A far better experience than Mingw is to use the Linux Subsystem for Windows 10. Full Ubuntu 16.04 LTS environment, but on Windows.

...

But also, I can see in file , lines 1267 to 1279

``` #ifdef __cplusplus namespace { #endif

typedef struct _IMAGEHLP_LINE64 { unsigned long SizeOfStruct; void *Key; unsigned long LineNumber; wchar_t *FileName; unsigned long long int Address; } IMAGEHLP_LINE64, *PIMAGEHLP_LINE64; ```

1. Why do you check for __cplusplus? Is it a copy-and-paste of some Windows headers?

It's C source code from an all C sub-sub-library brought into the partially preprocessed edition. That particular all C sub-sub-library implements POSIX backtrace() and backtrace_symbols() for Windows.

...

2. This class and subsequent functions are defined in anonymous namespace. This means they are redefined and recompiled in each translation unit, and remain a per TU definition. Is that intent?

Yes. Because it's a C source file, you would get symbol collision errors otherwise.

Can't the same effect be achieved with declaring the functions inline? And then you would only get one symbol. Regards, &rzej;

On Wed, May 24, 2017 at 7:28 AM, Niall Douglas wrote:

...

...

...

2. This class and subsequent functions are defined in anonymous namespace. This means they are redefined and recompiled in each translation unit, and remain a per TU definition. Is that intent?

Yes. Because it's a C source file, you would get symbol collision errors otherwise.

Can't the same effect be achieved with declaring the functions inline? And then you would only get one symbol.

We would then collide with anyone who later includes , because we are defining the exact same symbol names as windows in order to avoid including windows, so it would be an ODR violation.

It's a trick to include windows without including windows which only works on MSVC by taking advantage of its lax parser. That's why on clang we just go ahead and include windows.

Boost libraries already avoid including by leveraging Boost.WinAPI which provides the definitions necessary (and are added to as needed). Your use of an unnamed namespace for the types is questionable. Depending on the compiler, I think you should want to decorate them with __attribute__ ((__may_alias__)). Glen

On 25/05/2017 00:31, Niall Douglas wrote:

The code in question is in a sub-sub-library which is itself completely standalone and reusable. It is Windows only because it implements a common POSIX API for Windows, so such attributes won't be useful. And that code would only ever get used if someone didn't configure a better stack backtrace printer.

Speaking of, now that StackTrace has been accepted into Boost, are you considering migrating that code to use it instead? Somewhat related, the docs for error_code_extended::backtrace() specify that the returned value must be freed. Is that referring only to the array itself or must the individual char* within the returned char** be individually freed as well? Why doesn't this just return std::vectorstd::string or some kind of smart pointer for avoidance of doubt and exception-safety? It feels like this is allowing implementation details to escape unsafely.

Niall Douglas wrote:

As I have stated many times now, the stacktrace captured is 100% standard format and can be fed to any third party stack trace printer, including Boost.Stacktrace.

Stacktrace has two parts, one for printing, the other for capturing. The capturing part is essentially what you have, and during the Stacktrace review, I specifically insisted that it should be minimal, non-intrusive, and should not under any circumstances include . The idea was to make the capturing part of Stacktrace usable in header-only libraries such as, in out case, Outcome.

On 26/05/2017 00:57, Gavin Lambert via Boost wrote:

On 26/05/2017 02:42, Niall Douglas wrote:

...

...

Why doesn't this just return std::vectorstd::string or some kind of smart pointer for avoidance of doubt and exception-safety? It feels like this is allowing implementation details to escape unsafely.

The implementation literally returns what backtrace_symbols() returns, which is a malloced char **.

I assumed that, hence my comment about a leaking implementation detail. That's a reasonable return value for a C library like backtrace_symbols; it's bad style for a C++ library.

If you don't want the expense of copying the strings individually to a std::vectorstd::string, perhaps returning a std::unique_ptr would be a reasonable compromise for that specific issue, although that still masks the array-ness so it's not ideal.

That's a fair point. Logged to https://github.com/ned14/boost.outcome/issues/45.

The other problem with this API as it stands is that it provides no way to convey the actual number of frame strings returned. backtrace_symbols() in the underlying library makes no guarantee to null-terminate the array, as far as I can tell. raw_backtrace(), or backtrace() in the underlying library, does give you the number of frames filled but this information is not available to the caller of error_code_extended::backtrace() unless they call raw_backtrace() separately, which seems pointless.

And you've just discovered an exceeding bounds bug. Thank you very much. Logged to https://github.com/ned14/boost.outcome/issues/46.

Since symbolising the trace is inherently an expensive operation anyway I don't think it's worth worrying about avoiding the copy, which is why I suggested the std::vectorstd::string return value. It avoids both issues.

Agreed. Niall -- ned Productions Limited Consulting http://www.nedproductions.biz/ http://ie.linkedin.com/in/nialldouglas/