out_ptr review

From: David Sankel (camior_at_[hidden])
Date: 2019-06-27 14:06:44

# 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 <boost/out_ptr.hpp>
#include <sqlite3.h>
#include <iostream>

int main()
{
    std::shared_ptr<sqlite3> database;

    if (SQLITE_OK !=
        sqlite3_open("test.db",
                     boost::out_ptr::out_ptr(database, sqlite3_close))) {
        std::cout << "Problem opening the file";
    }

    using Sqlite3Ptr = std::unique_ptr<sqlite3, decltype(&sqlite3_close)>;
    Sqlite3Ptr database2(nullptr, &sqlite3_close);
    if (SQLITE_OK !=
        sqlite3_open("test.db",
                     boost::out_ptr::out_ptr(database2, sqlite3_close))) {
        std::cout << "Problem opening the file";
    }
}
```

With this `Makefile`

```c++
main : main.cpp
g++ -I/home/dsankel/code/out_ptr/include -o $@ $< `pkg-config sqlite3
--cflags --libs`

.PHONY: clean

clean:
$(RM) main
```

I didn't run into any issues. Everything worked okay.

I think this library would be handy for wrapping C libraries like sqlite.

# How much effort did you put into your evaluation of the review?

I spent a few hours on this review. I read through all the documentation,
did a cursory review of the code, and came up with the example above.

# Minor things

* Copyright notices need to have a person's real name to have legal
benefits.
* In `aout_ptr.adoc` the examples link goes to the overview page.
* Some grammar errors in the "C function calls are indeterminately
sequenced..." paragraph.
* When referring to a Boost library in free flowing text, the correct form
is Boost.LibraryName with the same font as the surrounding text.