Re: [review] [STLInterfaces] STLInterfaces review starts today

From: Hadriel Kaplan (hadrielk_at_[hidden])
Date: 2019-12-18 07:39:19

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