Re: [lmi] When should std::distance (not) be used?

[Vadim Zeitlin]

On Fri, 20 Jan 2017 10:21:17 +0000 Greg Chicares <address@hidden> wrote:

GC> AFAICT, std::distance() should be used only when truly needed.

 Yes, in generic code which needs to work with any kind of iterators.

GC> lmi uses std::distance() only twice. Here:
GC> 
GC> template<typename InputIterator, typename T>
GC> bool each_equal(InputIterator first, InputIterator last, T const& t)
GC> {
GC>     return std::distance(first, last) == std::count(first, last, t);
GC> }
GC> 
GC> it's unavoidable because we have no container, only iterators.

 Annoyingly, there is exactly one case in which we need to use iterators
here instead of passing the entire container as could be done for the 15
other uses of this function but it doesn't look like we can get rid of that
one (in custom_io_0.cpp).

 Also, this is slightly off topic, but this implementation of each_equal()
looks very strange to me because it doesn't "short circuit": we still loop
until "last" inside std::count() even if the very first element is
different from "t", which is clearly suboptimal -- but, of course, this
pessimization is completely unnoticeable for small containers.

GC> But
GC> in multiple_cell_document::parse(), where we push_back() each
GC> element of an xmlwrapp "container" into a std::vector:
GC> 
GC>     xml::const_nodes_view const subelements(i.elements());
GC>     v.reserve(std::distance(subelements.begin(), subelements.end()));
GC>     for(auto const& j : subelements)
GC> 
GC> shouldn't xml::const_nodes_view::size() be used instead of distance()
GC> -    v.reserve(std::distance(subelements.begin(), subelements.end()));
GC> +    v.reserve(subelements.size());
GC> because it's less verbose than distance(begin, end) and no slower?
GC> Or is there some subtlety I'm missing here?

 I think this is just because size() didn't exist in const_nodes_view when
you made this change in March 2012 in

https://github.com/vadz/lmi/commit/60b869773b3a257f5cae37acb9e083e17af89007

It was added by Vaclav 2 months later, see

https://github.com/vslavik/xmlwrapp/commit/9ae11570a20a6e9ae982a1a66d7b3010e846f0aa

and I think it's not a coincidence the times are so close, the latter
change must have been motivated by your earlier one -- but we've never
updated lmi code to use it.


GC> The related function std::advance() is used only once in lmi:
GC> 
GC>     std::vector<double>::const_iterator corr = 
CompleteGptCorridor().begin();
GC>     LMI_ASSERT
GC>         (   static_cast<unsigned int>(IssueAge)
GC>         <=  CompleteGptCorridor().size()
GC>         );
GC>     std::advance(corr, IssueAge);
GC>     GptCorridor.assign
GC>         (corr
GC>         ,CompleteGptCorridor().end()
GC>         );
GC> 
GC> and there I think it's better to do
GC> 
GC> -    std::advance(corr, IssueAge);
GC>      GptCorridor.assign
GC> -        (corr
GC> +        (corr + IssueAge
GC>          ,CompleteGptCorridor().end()
GC>          );
GC> 
GC> Making that less generic is IMO advantageous: changing these vectors to
GC> lists would be such a bad idea, that it's good to produce an error if
GC> anyone ever tries to do that.

 In this case you could also write

        GptCorridor.assign
            (&CompleteGptCorridor()[IssueAge]
            ,&CompleteGptCorridor().back() + 1
            );

which will be almost certain to not compile with vector or very vector-like
container. I.e. why use iterators at all if we want to intentionally make
the code non-generic?

 Regards,
VZ