On 24/01/2011, at 1:58 AM, Joseph Wright wrote:
> I'm happy with the idea in principal, but not the names or the
> implementation :-)
You had me worried there for a second, but your implementation is just an optimisation of mine -- phew! At least we're on the same wavelength there :)
Thanks for the feedback; exactly what I was hoping to hear.
> On the names, we currently have \tl_elt_count:N, so should really follow
> the model and have \clist_elt_count:N. On the other hand, I notice that
> there are various other uses of 'element' in other functions. So perhaps
> we'd be better having \tl_element_count:N and \clist_element_count:N,
> and not using 'elt' at all. (We also need :c and :n variants.)
If we were going to change the _elt_count one, wouldn't _length be the most sensible for all of them?
> For \clist_nth:N, I'd prefer '\clist_element:N'. We don't want 'get'
> here as that is used in non-expandable assignments (see \prop_get:NnN,
> etc.). 'nth' is really awkward, I think.
Sure, I agree here.
> On the implementation, there are some things to think about. You've gone
> for an implementation indexed from 1, which gives an error with 'out of
> range' cases. In other places we've indexed from 0 (as TeX does for
> \ifcase), and we need to cover negative numbers. I'd prefer to avoid two
> loops, and also think that 'out-of-range' values should simply result in
> an empty value, not an error.
Oh yes -- bailing at the end of the list rather than explicitly checking sizes is a fine idea. For negative indices, I think they should actually be used to access elements from the end.
> A couple of notes. I've gone for \int_compare:nNn for performance
> reasons over \int_compare:n,
Sure. Efficiency comes second after working implementation :)
> and I'd probably use the expansion of
> \int_eval:n in the 'count' functions, so that it only requires two
> expansions to get to the result rather than three.
You mean avoiding the \int_eval:n inside the 'f' expansion so you get "n-1-1-1-1-1-1-1-1==0" as the test goes along? I doubt it matters too much in terms of efficiency, and this makes the code more readable, so I've changed it to this approach.
* * *
Based on all this, I've updated the SVN repository with a new version of the code (but haven't done the seq versions, yet).