2015-07-04 19:31 GMT+03:00 Niall Douglas
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
- 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