# Whether you believe the library should be accepted into Boost
No. Not until some of the major issues are addressed.
# Your name
David Sankel
# Your knowledge of the problem domain
I've had to wrap C APIs on multiple occasions.
# What is your evaluation of the library's:
## Design
The design of the `out_ptr` type is reasonable as a shorthand for calling
certain C APIs.
In my experience a use case for `in_out_ptr` has never come up. Given the
subtleties of its correct (and likely rare) usage, I'd be in favor of
leaving this out of the library altogether. Especially given that the
semantics of when 'release()' is called are not specified.
I agree with the other comments that the customization point is not
minimally complete. Ideally a customization point would not require one to
replicate all the functionality of the class I'm customizing and, instead,
only require a few general purpose functions that that interface can make
use of.
## Implementation
There are two implementations of `out_ptr`. The one called "clever" invokes
undefined behavior as a means to squeeze out some extra performance. I do
not think the performance overhead of using this library with the "non
clever" implementation justifies the potential portability and and other
risks associated with depending on undefined behavior working a particular
way.
It looks like "clever" mode was #ifdef'ed out, but removing it as cruft
would improve maintainability of the implementation and remove the
complexity of having an additional `core_out_ptr_t` class.
The provided implementation works for smart pointers that follow
conventions like the standard, but has some additional logic if the types
happen to be `std::shared_ptr` or `boost::shared_ptr`. This hard coded
special casing won't work with, say, a company specific shared pointer
implementation. This relates to my earlier comment that the customization
point should be simplified.
## Documentation
The recommendation in the "caveats and caution" to not use out_ptr outside
if/else conditionals goes a bit far in my opinion. It is common practice to
make function calls that emit failure with return codes in conditionals, as
in:
```
if (SQLITE_OK !=
sqlite3_open("test.db",
boost::out_ptr::out_ptr(database, sqlite3_close))) {
std::cout << "Problem opening the file";
}
```
For these cases it is safe to use `out_ptr` so I'd suggest rewording the
recommendation to allow calls like this.
"Almost all well-designed C APIs will set the user-provided output pointer
argument to NULL/nullptr upon invalid parameters". This is a very difficult
claim to make (what's the sample size?). I'd suggest changing "Almost all"
to "Many". I don't understand what the issue is here with APIs not setting
the out parameter on failure (which IMO is good practice especially as it
relates to strong exception safety guarantees). These APIs have return
codes that need to be inspected anyway.
The reference documentation reads like a specification rather than what I
would typically consider reference documentation. I'd expect to see it
written with an intended audience of a casual user with examples and an
organization that would be helpful for this reader (e.g.: purpose,
description, examples, details)
## Tests
The tests look reasonable and comprehensive although there seems to be some
`#if 0` conditions that could either be cruft or perhaps the intent is the
developer would manually change those to verify compilation failure.
The tests are using the 'catch' library which is intended to be a
submodule. Is there a precedence for using a third party library like this?
For folks using Boost it could be painful to get another dependency
approved to run all the Boost tests. It would be preferable IMO to use
Boost.Test or something self contained.
## Usefulness
I attempted to use the library with g++ 8.3.0 and clang 8.0.0. My example
code is below:
```C++
#include