On Tue, Jun 16, 2015 at 8:04 AM, Louis Dionne <ldionne.2@gmail.com> wrote:
Zach Laine <whatwasthataddress <at> gmail.com> writes:
[...]
- I'd like to see first() / last() / after_first() / before_last() instead of head() / last() / tail() / init() (this was suggested by Louis offline), as it reinforces the relationships in the names.
Actually, I thought we had agreed on front/back drop_front/drop_back :-). Anyway, this is tracked by this issue [2]. It's not a 100% trivial change because that will mean that `drop` is replaced by `drop_front`, which can also take an optional number of elements to drop. Same goes for `drop_back`, which will accept an optional number of elements to drop from the end of the sequence.
That sounds good too.
- I find boost::hana::make<boost::hana::Tuple>(...) to be clunkier than boost::hana::_tuple<>, in many places.
You can use boost::hana::make_tuple(...).
Right. It's just that sometimes I just want to name the type.
- I ran in to a significant problem in one case that suggests a general problem.
In the linear algebra library I partially reimplemented, one declares matrices like this (here, I use the post-conversion Hana tuples):
matrix< hana::tuple<a, b>, hana::tuple<c, d>
m;
Each row is a tuple. But the storage needs to be a single tuple, so matrix<> is actually a template alias for:
template <typename Tuple, std::size_t Rows, std::size_t Columns> matrix_t { /* ... */ };
There's a metafunction that maps from the declaration syntax to the correct representational type:
template <typename HeadRow, typename ...TailRows> struct matrix_type { static const std::size_t rows = sizeof...(TailRows) + 1; static const std::size_t columns = hana::size(HeadRow{});
/* ... */
using tuple = decltype(hana::flatten( hana::make<hana::Tuple>( HeadRow{}, TailRows{}... ) ));
using type = matrix_t<tuple, rows, columns>; };
The problem with the code above, is that it works with a HeadRow tuple type that is a literal type (e.g. a tuple<float, double>). It does not work with a tuple containing non-literal types (e.g. a tuple<boost::units::quantity<boost::units::si::time>, /* ... */>).
This is a problem for me. Why should the literalness of the types contained in the tuple determine whether I can know its size at compile time? I'd like to be able to write this as a workaround:
static const std::size_t columns = hana::size(hana::type<HeadRow>);
That is the incorrect workaround. The proper, Hana-idiomatic way to write what you need is:
namespace detail { auto make_matrix_type = [](auto rows) { auto nrows = hana::size(rows); static_assert(nrows >= 1u, "matrix_t<> requires at least one row");
auto ncolumns = hana::size(rows[hana::size_t<0>]); auto uniform = hana::all_of(rows, [=](auto row) { return hana::size(row) == ncolumns; }); static_assert(uniform, "matrix_t<> requires tuples of uniform length");
using tuple_type = decltype(hana::flatten(rows));
return hana::type<matrix_t<tuple_type, nrows, ncolumns>>; }; }
template <typename ...Rows> using matrix = typename decltype( detail::make_matrix_type(hana::make_tuple(std::declval<Rows>()...)) )::type;
While that definitely works, it also definitely leaves me scratching my head. Why doesn't static const std::size_t columns = hana::size(HeadRow{}); Work in the code above, especially since auto ncolumns = hana::size(rows[hana::size_t<0>]); does? That seems at least a little obscure as an interface. I also could not get hana::size(hana::type<HeadRow>); to work, btw. By the way, see my pull request at [4]. I'm now passing all the tests, and
with a compilation time speedup!
Very nice!
As an aside, note that I was forced to name this nested '_' type elsewhere in a predicate. This is not something I want my junior coworkers to have to read:
template <typename T> constexpr bool some_predicate (hana::_type<T> x) { return hana::_type<T>::_::type::size == some_constant;}
You made your life harder than it needed to be. First, you should have written
`decltype(hana::type<T>)::type::size`
instead of
`hana::_type<T>::_::type`
This `_` nested type is an implementation detail. But then, you realize that this is actually equivalent to `T::size`. Indeed, `decltype(type<T>)::type` is just `T`. So basically, what you wanted is
template <typename T> constexpr bool some_predicate (hana::_type<T> x) { return T::size == some_constant;}
which looks digestible junior coworkers :-).
Ha! Fair enough.
That being said, I really, really, want to do this:
tuple_1 foo; tuple_2 bar;
boost::hana::copy_if(foo, bar, [](auto && x) { return my_predicate(x); });
Currently, I cannot. IIUC, I must do:
tuple_1 foo; auto bar = boost::hana::take_if(foo, [](auto && x) { return my_predicate(x); });
Which does O(N^2) copies, where N = boost::hana::size(bar), as it is pure-functional. Unless the optimizer is brilliant (and it typically is not), I cannot use code like this in a hot section of code. Also, IIUC take_if() cannot accommodate the case that decltype(foo) != decltype(bar)
I don't understand; take_if() is not an algorithm provided by Hana.
Right! I meant filter(). Sorry.
What I ended up doing was this:
hana::for_each( hana::range_c<std::size_t, 0, hana::size(foo)>, [&](auto i) { hana::at(bar, i) = static_cast<std::remove_reference_t<decltype(hana::at(bar, i))>>( hana::at(foo, i) ); } );
... which is a little unsatisfying.
I also want move_if(). And a pony.
Oh, so you're trying to do an element-wise assignment?
Yes.
Since Hana expects functions to be pure in general and assignment is a side effect, this cannot be achieved with a normal algorithm. However, Hana provides some algorithms that alow side-effects. One of them is for_each, and the other is `.times` (from IntegralConstant). I suggest you could write the following:
hana::size(foo).times.with_index([&](auto i) { using T = std::remove_reference_t<decltype(bar[i])>; bar[i] = static_cast<T>(foo[i]); });
Ok. You also had this in the PR you made for Units-BLAS: hana::size(xs).times.with_index([&](auto i) { xs[i] = ys[i]; }); ... which is even simpler. However, either is a pretty awkward way to express an operation that I think will be quite often desired. copy(), copy_if(), move(), and move_if() should be first-order operations. I also want the functional algorithms, but sometimes I simply cannot use them.... Now where's my pony?
- Did you attempt to use the library? If so: * Which compiler(s)
Clang 3.6.1, on both Mac and Linux. My code did actually compile (though did not link due to the already-known bug) on GCC 5.1.
Cool!
* What was the experience? Any problems?
I had to add "-lc++abi" to the CMake build to fix the link on Linux, but otherwise no problems.
That is curious, because the Travis build is on linux and does not need that. Did you set the path to your libc++ installation on Linux? The README states that you should use
-DLIBCXX_ROOT=/path/to/libc++
Ah, cool. I don't think I read the README.
Thanks for the review, Zach. I also appreciate all the comments you provided privately; they were super useful. And you were right about the dots in the names; I should have changed them before the review. :-)
Glad to help! Zach