Lorenzo Caminiti
[...]
* [CONCERN] Assertions should be renamed from ..._CHECK to ..._ASSERT (to be consistent with assert, static_assert, BOOST_STATIC_ASSERT, BOOST_MPL_ASSERT, etc.).
There are actually `BOOST_HANA_..._ASSERT` macros. However, the _ASSERT versions are disabled in release mode, while the _CHECK versions are not. Since I want the conditions to be checked at all times in the examples and unit tests, I use the _CHECK version.
* Is there a ..._MSG version for the assertion macros? (These should probably just variadic macros where a second optional parameter represents the error message to display in case the asserted condition if false.)
No, there are no such macros. Actually, I'm not sure whether those assertion macros should be part of the library's interface at all. Indeed, there is no perfect way to implement them and I think it would be better if they were not used by users. If so, I should move them to detail/ and document that they just give semantic information about the nature of the condition for the documentation.
[...]
BTW, how would you program the following possible implementation of std::advance in Hana?
[...]
I might write something like this:
template
* This macro:
struct Person { BOOST_HANA_DEFINE_STRUCT(Person, (std::string, name), (int, age) ); };
Will it work even if the data member types contain commas? [...]
Yes, it will also work.
* [CONCERN] The docs say: "Hana's design assumes that most of the time, we want to access all or almost all the elements in a sequence anyway, and hence performance is not a big argument in favor of laziness." Especially given that other well-established libraries like Fusion addressing the same problem domain have gone in favor of laziness instead... Is this a good assumption? If so, why?
This assumption is based mainly on two things: 1. From my personal experience, I don't remember ever relying on Fusion's lazy semantics. Admittedly, this is a poor rationale. However... 2. For validation, I asked my GSoC mentor, Joel Falcou, whether the lazy semantics were an important part of Fusion that should be preserved. The answer was basically that it made the implementation of Fusion easier, but that it was not a crucial aspect. Also, we couldn't take advantage of C++11/14 features as much with lazy semantics (in particular parameter packs) to make the compile-times better. So it would have been a C++11/14 reimplementation of Fusion, without much design changes. I think time will tell whether this was a good idea, but in all cases it would be possible to add lazy views to Hana in the future if we realize this was a mistake.
* [CONCERN] The docs say: "By default, Hana assumes functions to be pure. A pure function is a function that has no side-effects at all." Will this assumption limit what users will be able to do with Hana?
To be pedantic, no, because of Turing completeness. In practice, I also do not think it will be a problem. However, this is really a "FP vs the world" thing. Some things will be better expressed functionally, and some things will be better expressed with mutation, but that's an old debate. Hana has gone on the road of purity, but I introduced loopholes in the system to make trivial stuff achievable without conceptual overhead. This is the case of `for_each`, for example, which takes a function that can have side effects. It also guarantees the order in which the function is called, as you would expect. Without this "loophole", we would need to use Monads to achieve side effects. I thought this was overkill and would hinder the adoption of the library in a C++ context. In summary; you do almost everything with pure functions, and when it becomes painful you have loopholes that allow mutation and side effects.
Do MPL and Fusion relay on a similar assumption? Yes/no, why...
For MPL, this is not relevant because it is type-level computations only.
Regarding Fusion, I don't think there are any mentions of whether the
functions are allowed to have side effects. For example, is the following
well-defined?
fusion::vector
[...]
* [CONCERN] Rename Map, Range, etc. to map, range, etc. (all lower case). In Boost and STL capital letters are only used for template parameters, and maybe concepts. (If it is important to distinguish that those are tags (I don't think so) then name them map_tag, range_tag, etc. but still no capital letters.)
This was already suggested a couple of times. Added an issue [2] to consider it.
* [CONCERN] is_empty, length, and few other names should be changed to empty, size, etc. to match STL naming conventions (so I don't have to remember different names for the same thing).
You can use `size` in place of `length` if you prefer; an alias is provided for consistency with the STL. Regarding `empty` vs `is_empty`: `empty` is already used to denote an empty container (`empty<Tuple>()`, `empty<Optional>()`, etc..) Furthermore, regardless of the choices made in the STL, I find `is_empty` to be arguably more descriptive than `empty`. I try to have some consistency with the STL, but I think diverging is OK when I judge a STL name to be really bad.
[...]
* Documentation
* [CONCERN] The documentation will be much (much!) more readable is the "hana" namespace was not stripped away. Please write hana::..., mpl::... (if not the full boost::hana::..., boost::mpl::...) in the docs.
This will be changed. See this issue [3].
* The docs say: "Notice how we cast the result of x.member to void? This is to make sure that our detection also works for types that can't be returned from functions, like array types."
auto has_member = is_valid([](auto&& x) -> decltype((void)x.member) { });
But obviously, the following (without the cast) would work too, correct?
auto has_member = is_valid([](auto&& x) -> decltype(x.member, void()) { });
Yes, I would expect the second to work as well.
* I would expect the following example to use operator|| instead of or_:
auto ts = filter(types, [](auto t) { return or_(is_pointer(t), is_reference(t)); });
Currently, the traits return std::integral_constants, not Hana's integral constants. However, this is part of this issue [4].
* Performance graphs would be more readable if they always used the same color for the same entity. For example, sometimes mpl::vector is blue, other times it is orange...
You're right. See this issue [5].
* Why the "Runtime performance of reverse on normal container" for fusion::vector stops at "Number of elements" = 50?
Because you can't create a fusion::vector with more than 50 elements.
[...]
* [CONCERN] The author should be committed to support the library on GCC and MSVC once that becomes feasible,
Yes.
even if that requires a good deal of workarounds.
No. I'm coding against a standard, not a specific compiler. If I introduce workarounds in Hana, the whole purpose of the library is lost. Performance, ease of use, and most importantly hackability will be lost. Quite honestly, I'd rather have my library never get into Boost than bastardize it so it works on MSVC. If it can be made to work on MSVC without bastardizing it, however, I'm all for it.
[...]
Maybe MSVC will never be 100% C++14 standard compliant... I don't know. But I do know that if this library is not ported to MSVC at some future point, I won't be able to use it in my production code.
Anyone is welcome to try and create a MSVC compliant fork of the library. If it works out well, I can guarantee I'll merge everything in the main branch.
I do hope that adoption into Boost will push vendors to support C++14 constructs used by this library TMP techniques.
I hope and think it will (if the library is accepted).
I also hope the author is committed to do whatever it takes to make the library work on MSVC.
* [CONCERN] What compiler was used to measure performance? CLang? I hope the promised performance gains (especially compile-time reductions) will be there for GCC, and especially later also for MSVC. If not, that will significantly limit my motivation on using this library in production code.
So far, performance gains were measured on Clang. However, older benchmarks I made for MPL11 were also on GCC, and they showed similar gains. So I think the modern techniques used in Hana will provide a speedup on both GCC and Clang. Once GCC is fully supported, I will also run benchmarks on GCC and make them available in the documentation.
TMP compilation times are largely dependant on specific compiler implementations of C++ template machinery which might vary significantly from vendor to vendor. Therefore, at the moment I only know that the library is expected to compile faster on Clang, but unfortunately that does not tell me anything about expected compilation time improvements on GCC, and even less on MSVC...
We'll be able to have different algorithm implementations on different compilers if there's a need for it. So in all cases, I expect Hana should be able to beat anything you'd write by hand (naively).
- Did you attempt to use the library? If so: * Which compiler(s)?
Clang.
* What was the experience? Any problems?
Overall, pretty good experience. No specific issue to report a part from what already noted.
- How much effort did you put into your evaluation of the review?
A total of about 2 days between reading docs, playing with some examples, and looking at the code a bit.
My thanks to Luis for his outstanding work with this library!
--Lorenzo
Thanks a lot for the review, Lorenzo. Regards, Louis [1]: http://goo.gl/MPiZx0 [2]: https://github.com/ldionne/hana/issues/157 [3]: https://github.com/ldionne/hana/issues/126 [4]: https://github.com/ldionne/hana/issues/92 [5]: https://github.com/ldionne/hana/issues/151