Re: [boost] Boost.DLL formal review is ongoing

Subject: Re: [boost] Boost.DLL formal review is ongoing
From: Antony Polukhin (antoshkka_at_[hidden])
Date: 2015-07-05 06:42:33

2015-07-04 19:31 GMT+03:00 Niall Douglas <s_sourceforge_at_[hidden]>:

> On 3 Jul 2015 at 16:50, Vladimir Prus wrote:
>
> > - Should the library be accepted?
>
> Yes, conditional on the items below.
>
> > - How useful is it?
>
> It is useful, though not as useful as it could be.
>
> > - What's your evaluation of
> >    - Design
>
> The design is conservative and unsurprising, and is very similar to
> probably what 90% of us here would have chosen, myself included if
> one was tasked with an uncontroversial solution. I cannot fault it.
>
> My only qualm really, and this is entirely my fault, is there is a
> lack of type safety when loading symbols to a given type. This is my
> fault because I had said I would write a demangler which could check
> types matched, and I haven't had the time. If this feature existed,
> it could detect type mismatches and therefore ABI mismatch rather
> than just hoping a segfault should happen.
>
> A poor man's implementation could instead do demangle both symbols
> into a string, and do a nasty platform specific regex transformation
> into strings which are comparable. Slow and inaccurate though
> compared to a demangler based design.
>

Symbol demangling is not a silver bullet. Following functions and variables
may have the exactly same names in shared objects (and most of them have in
GCC!):

extern "C" {
BOOST_SYMBOL_EXPORT void foo(int, char*);
BOOST_SYMBOL_EXPORT int foo;
}
BOOST_SYMBOL_EXPORT void foo();
BOOST_SYMBOL_EXPORT boost::variant<int, short, std::string> foo;
BOOST_SYMBOL_EXPORT class foo {};

The only available improvement is to detect section permissions for an
exported symbol. If symbol is in executable section - then it is probably a
function (no 100% guarantee), If symbol is in read-only section, then it is
probably a constant(no 100% guarantee). If symbol in read-write section -
then it is probably a variable(no 100% guarantee).

> >    - Implementation
>
> I don't see any good reason why this library needs to be dependent on
> Boost. It uses little in Boost not also present in the C++ 11 STL,
> the only major blocker I noticed is some limited use of the MPL which
> is easily replaced with minimal constexpr as supported by VS2015.
>
> CONDITION: It should therefore support standalone usage decoupled
> from the rest of Boost on C++ 11 compilers. It can still use Boost on
> C++ 98 compilers.
>

I'd rather not do a standalone version. If everything goes OK, I was
planning to make two C++ proposals:
* [[export]], [[export "valid_c_name"]], [[import]], [[import
"valid_c_name"]]
* shared_library (without all the *alias() magic, because [[export
"valid_c_name"]] makes a true alias) + library_info

So I think it's better to concentrate on a proposals and get the superior
version in next Standard then adding new unrelated features to prototype.

CONDITION: Namespace is not inline versioned on C++ 11 compilers. It
> should be.
>

+1. But I'd rather emulate them:

namespace boost { namespace dll {
    namespace v1 {
        // all the code
    } // namespace v1

    using namespace v1;

}} // namespace boost::dll

Otherwise implementation is very solid. I wouldn't expect any
> different from Antony.
>

Thanks :)

>
> >    - Documentations
>
> CONDITION: BoostBook pages are missing badges for the CI test status.
> Readme has them though.
>

Will be fixed.

> CONDITION: No ability to launch example code in online web compiler.
>

I few days before the review I've started experimenting with online
compilation of  source codes from "Boost C++ Application Development
Cookbook". I've stopped on http://coliru.stacked-crooked.com/ service. They
have REST API, so it's possible to directly embed examples in documentation
and allow users to modify them:
http://en.cppreference.com/w/cpp/utility/tuple (See the "Example" on the
bottom of the page).

After I end the experiments on "Boost C++ Application Development
Cookbook", I'll probably do something with the DLL docs (and with
LexicalCast, TypeIndex, Variant ...)

> There is no acknowledgements section, and for good form there
> probably should be.
>

+1

> Otherwise very good. I think I had sent Antony a list of docs
> problems last year, and he must have fixed them because I spotted
> nothing which particularly bothered me (that other reviews haven't
> already reported).
>
> >    - Tests
>
> CONDITION: No Appveyor integration.
>

Have not seen this service before. I'll take a look at it.

> CONDITION: I saw a Coverity scan, but no clang-tidy + clang static
> analyser. A MSVC static analyser pass would do no harm either.
>

There are now regression testers in Boost that run static analysers:
http://www.boost.org/development/tests/develop/developer/summary.html
Adding additional auto test runners may be an overkill.

> CONDITION: The unit tests are not run under valgrind memcheck by
> default.
>

I do not know how to make tests run with valgrind by default without
affecting regression testers that do not have valgrind installed.

> Travis doesn't seem to have OS X testing enabled. The docs mention OS
> X isn't working yet, so that is understandable.
>

They have a few OS X guests and a huge queue of projects that require OS X.
I'm somewhere in the middle of the queue.

> Tests seem more functional than unit testing, but that is acceptable.
> I'm the same in AFIO.
>
> > - How much effort did you put into your evaluation?
>
> About an hour, though I have been watching the development for a long
> time. I even promised some code I failed to deliver upon :(
>
> > - Did you attempt to use the library? On what systems and compilers?
>
> Not on this occasion, but I did last year on Linux and Windows. It
> worked fine then.
>

Thanks for the review!

-- 
Best regards,
Antony Polukhin