The formal review of Boost.DLL library by Antony Polukhin starts today. Boost.DLL is a C++98 library for comfortable work with DLL and DSO. Library provides a portable across platforms way to: - load libraries at runtime - import and export any native functions and variables - make alias names for C++ mangled functions and symbols - query libraries/objects and executables for sections and exported symbols - self loading and self querying - getting program and module location by exported symbol The documentation can be found at: http://apolukhin.github.io/Boost.DLL/index.html and the source can be obtained at: https://github.com/apolukhin/Boost.DLL Please post your reviews on the mailing list and if possible, answer the following questions: - Should the library be accepted? - How useful is it? - What's your evaluation of - Design - Implementation - Documentations - Tests - How much effort did you put into your evaluation? - Did you attempt to use the library? On what systems and compilers? This formal review has a couple of adjustments in place: - It will run for two calendar weeks, not for 10 days, as a number of people said 10 days is too little. That said, please try to submit a review during first week, so that necessary discussion can happen conveniently. - The documentation site has Google Analytics enabled, which will allow us to see which pages and navigation paths are most useful for reviewers. No personally identifiable information will be collected. I will post the data after the review is over. Thanks, Volodya
On 29 June 2015 at 15:11, Vladimir Prus <vladimir.prus@gmail.com> wrote:
The documentation can be found at:
Just a quickfix while I'm reading it: In http://apolukhin.github.io/Boost.DLL/boost_dll/tutorial.html#boost_dll.tutor... "// This block is invisible for Quickbook documentation All you need to do, is write a simple class that stores callbacks and calls them at destruction:" I believe the first sentence should not have been visible.
2015-06-29 20:10 GMT+03:00 Klaim - Joël Lamotte <mjklaim@gmail.com>:
On 29 June 2015 at 15:11, Vladimir Prus <vladimir.prus@gmail.com> wrote:
The documentation can be found at:
Just a quickfix while I'm reading it:
In
http://apolukhin.github.io/Boost.DLL/boost_dll/tutorial.html#boost_dll.tutor...
"// This block is invisible for Quickbook documentation All you need to do, is write a simple class that stores callbacks and calls them at destruction:"
I believe the first sentence should not have been visible.
Fxd, thanks! -- Best regards, Antony Polukhin
On 29 June 2015 at 22:54, Antony Polukhin <antoshkka@gmail.com> wrote:
2015-06-29 20:10 GMT+03:00 Klaim - Joël Lamotte <mjklaim@gmail.com>:
On 29 June 2015 at 15:11, Vladimir Prus <vladimir.prus@gmail.com> wrote:
The documentation can be found at:
Just a quickfix while I'm reading it:
In
http://apolukhin.github.io/Boost.DLL/boost_dll/tutorial.html#boost_dll.tutor...
"// This block is invisible for Quickbook documentation All you need to
do,
is write a simple class that stores callbacks and calls them at destruction:"
I believe the first sentence should not have been visible.
Fxd, thanks!
Another quick note: you might want to clarify somewhere that this is a header-only library. "To start with the library you only need to include <boost/dll.hpp> header. " is useful but doesn't state exactly the same and I had to look in the repo to check that I didn't have to compile it. Otherwise the documentation is nice. I'm preparing a review.
On 06/29/2015 03:11 PM, Vladimir Prus wrote:
- Should the library be accepted?
Yes.
- How useful is it?
It hides most of the complexity if you want to build a plugin framework.
- What's your evaluation of - Design
I like how reference counting can be used to manage life-times. It is unclear to me whether there are any limitations about aliasing. For instance, can I alias constructors, operators, or functions in an anonymous namespace?
- Implementation
Looks clean from my brief examination. My only complaint is that some headers includes system headers that typically brings in their own macros and non-namespaced types which may or may not colide with the user code. For example, <boost/dll/runtime_symbol_info.hpp> includes <dlfnc.h> on Posix platforms. This is not a major complaint, and I realize that this can be difficult to avoid in a header-only library.
- Documentations
Generally good and well-structured. Some suggestions and minor comments: Some sentences are difficult to read due to their grammar. For example, the "Linking plugin into the executable" tutorial, or the the second and third questions in the FAQ. Tutorial "Searching for a symbol in multiple plugins" should mention that previously loaded libraries are unloaded on subsequent loads (this is mentioned in the reference documentation, but it would be nice to have here as well.) Tutorial "Symbol shadowing problem (Linux)" should start by describing the problem it solves. Reference BOOST_DLL_SECTION says: ''Permissions Can be "read" or "write" (without quotes!).'' I suggest that you list the names as code-blocks instead (e.g. use `read` in the .qbk) Reference runtime_symbol_info: When I first saw the *_location names I thought that they referred to the addresses (especially this_line_location.) I suggest that they be renamed to *_path. Reference dll::import and dll::import_alias uses string_ref for func_name, yet states that func_name must be a "Null-terminated C or C++ mangled name of the function to import. Can handle std::string, char*, const char*." FAQ ends with "[endsect]" The design rationale and the FAQ seems to be in disagreement about ABI portability.
- Tests
Did not look.
- How much effort did you put into your evaluation?
A couple of hours reading the documentation and browsing the code.
- Did you attempt to use the library? On what systems and compilers?
No.
2015-07-11 14:51 GMT+03:00 Bjorn Reese <breese@mail1.stofanet.dk>:
It is unclear to me whether there are any limitations about aliasing. For instance, can I alias constructors, operators, or functions in an anonymous namespace?
Functions and classes from anonymous namespaces could be exported using aliases. There's no anonymous namespace related restrictions.
- Implementation
Looks clean from my brief examination.
My only complaint is that some headers includes system headers that typically brings in their own macros and non-namespaced types which may or may not colide with the user code. For example, <boost/dll/runtime_symbol_info.hpp> includes <dlfnc.h> on Posix platforms. This is not a major complaint, and I realize that this can be difficult to avoid in a header-only library.
This is avoided for Windows (we have the Boost.Winapi module that takes care of that), but for Linux, MacOS and other platforms this could be an issue. Extremely heavy headers are avoided (for examples those, that contain platform specific binary formats descriptions). Taking care of others requires new module Boost.POSIX, which is not in my plans right now.
- Documentations
Generally good and well-structured.
Some suggestions and minor comments:
Some sentences are difficult to read due to their grammar. For example, the "Linking plugin into the executable" tutorial, or the the second and third questions in the FAQ.
Tutorial "Searching for a symbol in multiple plugins" should mention that previously loaded libraries are unloaded on subsequent loads (this is mentioned in the reference documentation, but it would be nice to have here as well.)
Tutorial "Symbol shadowing problem (Linux)" should start by describing the problem it solves.
Reference BOOST_DLL_SECTION says: ''Permissions Can be "read" or "write" (without quotes!).'' I suggest that you list the names as code-blocks instead (e.g. use `read` in the .qbk)
Reference runtime_symbol_info: When I first saw the *_location names I thought that they referred to the addresses (especially this_line_location.) I suggest that they be renamed to *_path.
Reference dll::import and dll::import_alias uses string_ref for func_name, yet states that func_name must be a "Null-terminated C or C++ mangled name of the function to import. Can handle std::string, char*, const char*."
FAQ ends with "[endsect]"
I'll fix those issues.
The design rationale and the FAQ seems to be in disagreement about ABI portability.
Did not get that. At what place do they disagree? ABI != portability. -- Best regards, Antony Polukhin
On 07/11/2015 11:50 PM, Antony Polukhin wrote:
The design rationale and the FAQ seems to be in disagreement about ABI portability.
Did not get that. At what place do they disagree? ABI != portability.
On what side of that equation does "ABI portability" fit? This may be the source of the confusion. As I currently read the documentation, the design rationale leaves the impression that there is no ABI portability, whereas the FAQ that there is some (e.g. for aliases.)
participants (4)
-
Antony Polukhin -
Bjorn Reese -
Klaim - Joël Lamotte -
Vladimir Prus