# 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 #include #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; 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.