Howdy, For this review I've really only tried out the iterator_interface part. I'm sorry I can't provide feedback on the container_interface nor view_interface, as I just don't have the free cycles.
- Your name
Hadriel Kaplan
- Your knowledge of the problem domain
I've been using boost's iterator_facade and adaptor for a long time; I've written my own custom iterators as well, of both traditional and proxy-types. The usual stuff.
- Whether you believe the library should be accepted into Boost (be clear about this)
Definitely. It could use word-smithing and minor cleanup stuff, but presumably that happens later. (?)
- What is your evaluation of the library's * Design?
Good overall.
* Implementation?
Good. I've got some nitpicks, but honestly none are a big deal. minor nitpicks: 1) I didn't use the proxy_iterator_interface one, only looked at the code. The proxy_arrow_result storing a local T value that it only returns a pointer to, means it's not optimal in some cases. Obviously for something like std::vector<bool> it won't matter, but some value-types are not so lightweight. On the other hand, it looks like we can specialize this for our type (or not even use proxy_arrow_result to begin with)? 2) Can the stuff in fwd.hpp be split out? I could be wrong, but I don't think most of it is needed/used by iterator_interface, and only used by the other two, correct? Reducing what gets included would be nice, for people who don't need the container/view stuff. The old boost iterator_facade/adaptor headers included so much that it drove some people to stop using them; for example see the comments here: https://github.com/facebook/folly/blob/master/folly/detail/Iterators.h 3) This lib hasn't been tested for C++20, and obviously can't be yet, but it makes assumptions that it can safely replace some of its internals with C++20's new ones. Things like concepts and ranges. I'm not sure that's advisable, until all the major compilers support them and can be tested. It would be a shame if people using this lib can't upgrade to C++20 because it breaks this lib, until they upgrade boost as well. (not a big deal, just more noting it) 4) The files use a macro to control doxygen output. Is this still required for boost libs? Doxygen has had the cond/endcond commands for a long, long time. It's kinda ugly to see these macros, and kinda silly that everyone's compiler will be evaluating them all the time. (I know that's super-nitpicky) 5) I assume all the _has_include(<stl2/ranges.hpp>) checks will be removed, since that's a thirdparty header? 6) This lib requires C++14, but I don't see a macro checking that anywhere and providing a simple error to the user. 7) I can't decide if the BOOST_STL_INTERFACES_STATIC_ASSERT_ITERATOR_TRAITS() thing is a good idea or a waste of time. I tried it, it passed. But of course it did, because it's just checking that I got the API that I told iterator_interface to create, no? This just seems like needless repetition to me. I mean if I screwed it up to begin with, I'll screw it up in this macro too, no? But maybe I'm missing something obvious. (could be - it's getting late)
* Documentation?
It's OK. I enjoyed some of the humor. :) It's missing some info for things that are customization points. For example base_reference() is shown in an example, but not really documented anywhere as far as I can tell. The tables aren't as useful/explanatory as one would hope. I mean I knew what they meant, but I'm not sure someone newer to this area would. (but then again, that's what blogs/articles are for I guess) I think it might also help if there were some explanation for what happens under-the-hood, in terms of what public methods get auto-generated from what, for which iterator types. For example, it's surprising that for a bidirectional iterator the user defines operator==(), but not for random-access iterator. (that particular case happens to be mentioned in a note, but you get my point) There are also typos and such, but I presume that gets checked later.
* Tests?
Good. I copied them over and ran them as well. (but not the v2 tests, nor the ones for container_interface or view_interface) I think it would be better if there were some tests for iterator_interface with non-copyable value-types. Testing it only for `int`s is cheating. :) Also, random question: for the container_interface, did you try using it with the boost::fixed_string (or static_string, or whatever it's called)? I mean that _is_ a contiguous container, and it's in boost review too, so might it be a good test candidate? :)
* Usefulness?
Very useful. iterator_facade/adaptor were showing their age. It's time for something new and shiny. I was happy that it reduced the number of functions we needed to implement in our current usages from 5 to 3.
- Did you attempt to use the library?
Yup. I replaced three existing boost::iterator_facade usages in my company's code-base with this one, to try it out. Ran our unit tests with and without valgrind as well. But to be honest these weren't very complicated iterators. I did _not_ try replacing boost::iterator_adaptor, but I hope to try that eventually.
* Which compiler(s)?
(old ones, so I could build my employer's codebase and run all their unit tests...) 1) gcc 7.3.1 with c++17 2) clang 5.0.0 with C++17, using gcc's libstdc++ Both using boost 1.55 (sorry!), with the stl_interfaces headers copied over manually, without using the CMake files in the github repo. Neither compiler has the early Concepts support.
* What was the experience? Any problems?
Not really. The hardest part was figuring out the new model of what the derived had to define, compared to iterator_facade. (i.e., I was so used to the old model that it took me a bit to grok this one)
- How much effort did you put into your evaluation of the review?
Only a few hours, maybe 4-5 total. Sorry, I wish I had more free time. :(
- Do you like the name container_interface? sequence_container_interface is more precise, but seems a bit long.
I'm not sure the length matters much for this use-case, as it's a one-time thing (generally), and not used by users of the actual container; and can anyway be aliased. (I mean, the "stl_interfaces" sub-namespace is annoying too, but it's not a big deal in my opinion)
- Would you use something like container_interface, but for associative containers? If so, should it assume a node-based interface, a la std::map and std::set? This assumption would preclude alternative associative container-like types, such as flat_map.
Yes I think so, depending on what it did... but for *unordered* ones (i.e., hash-maps/sets), rather than rb-trees. I often find myself encapsulating them, and then having to write a lot of boilerplate public member functions. And several functions can be boiled down for hash-maps. For example count() and contains() can just use find(). I don't know why an unordered_container_interface wouldn't be do-able, including heterogenous lookups, for various hash-map/set flavors. (except, of course, that boost::unordered_map/set's API is different than std's for heterogeneous lookup; which is unfortunate) -hadriel