Re: [boost] [Hana] Informal review request

5 Mar 2015


      Steven Watanabe  writes:
...
AMDG
On 03/05/2015 12:02 PM, Louis Dionne wrote:
...
Steven Watanabe  writes:
...
I've just glanced over the main page of the documentation
and I had a few comments:
- _tuple
  Historically names beginning with _ are used for placeholders.
  i.e. in Boost.Bind/Lambda/Phoenix/Parameter.  Is there
  a good reason not to call it tuple?
It is because of other constructs in Hana like `tuple_t` 
which are variable templates and whose type is not specified. I have 
not found a good naming convention for variable templates and types yet. 
I'll sketch out the current state of affairs, and perhaps someone can 
suggest a convention that would work well.
I see.  You have too many similar names.
I'm not fond of it, but I can't think of
anything better off the top of my head.
...
...
- // BOOST_HANA_RUNTIME_CHECK is equivalent to assert
  In that case, what is the point of using it instead of assert?
  <snip>
The problem is that `assert` is not define when NDEBUG is defined, 
but I want my tests to fire up all the time. Since these examples 
are Doxygen'd out of actual source files, the macros end up in the 
documentation. Basically, If I could redefine `assert` to be 
`BOOST_HANA_RUNTIME_CHECK` in those test files, I'd be really 
happy. Would it be against good software craftsmanship to write
... include all you need ...
#ifndef assert
    #   define assert(...) BOOST_HANA_RUNTIME_CHECK(__VA_ARGS__)
    #endif
It won't work.  assert is still defined
with NDEBUG.  It's just defined to do nothing.
Yes, of course, I know that. I don't know why I proposed that.
Well, I could undefine assert and redefine it to my own macro,
but that's probably a bad idea so I won't do it.
...
Personally, I'd just use assert and make
sure that NDEBUG is not defined.  It isn't
really important though, and I don't think
it's a good idea to put ugly workarounds
in examples, since the whole point of
examples is to be relatively easy to understand.
I just thought about another role of BOOST_HANA_RUNTIME_CHECK.
It also gives me the guarantee that the expression that's being 
asserted upon is not an IntegralConstant. It can become very hard 
to track the IntegralConstantness of things because of their 
implicit conversion to their underlying value, so the assert 
macros also make sure I'm expecting the right thing. The type
of assertion I use in the documentation is actually a great 
source of clarity as to what can be expected from the library.
For example: (untested)

    // can only be known at runtime
    std::vector<int> vs{1,2,3};
    BOOST_HANA_RUNTIME_CHECK(!vs.empty()); 

    // can be known at compile-time with an IntegralConstant
    auto i = int_<1>; //
    auto j = int_<3>; // notice no constexpr 
    BOOST_HANA_CONSTANT_CHECK(i != j);

    // can be known at compile-time, but not with an IntegralConstant.
    // it requires the arguments to be constexpr.
    constexpr int i = 1;
    constexpr int j = 3;
    static_assert(i != j, "");

    // Would be equivalent to static_assert if constexpr lambdas were 
    // supported. In other words, it's not constexpr but that's just
    // an implementation defect, not an intrinsic limitation.
    constexpr auto xs = tuple_c;
    BOOST_HANA_CONSTEXPR_ASSERT(sort(xs) == tuple_c);


That being said, I agree 100% with you that the documentation 
should be devoid of unneeded tricks. I'm unsure whether I should
just use assert or explain the different kinds of assertions. 
One problem with explaining them is that it requires a good
understanding of foundational concepts that are covered at the
end of the tutorial. I can't put these foundational concepts 
at the beginning of the tutorial, because people would run away
screaming. Now you know my dilemma.
...
...
...
- count is the equivalent of std::count_if, not std::count.
- find/lookup should be find_if/find
You're right. It's sometimes hard to set aside personal
preferences for the greater good, but I'll do it .
I wouldn't really object if you only
provided predicate forms.  The problem
is that some algorithms have only a
predicate form, while others have
both predicate and value forms.  If
you do provide both, than you need
a consistent naming convention, and
while f/f_if aren't the greatest
names ever, they will be familar to
most C++ programmers.
I understand. I have to provide both predicated and
value-based forms because the value-based forms allow
some rather heavy optimizations.
...
...
...
- In general I would expect any algorithm that takes
  a single element for comparison to have both
  f and f_if forms.
It is possible that a structure can do lookup by equality but not 
by any predicate, so it wouldn't be able to provide both.
That seems like a serious limitation.
Logically find(s, x) should be equivalent
to find(s, [](auto y) { return x == y } )
Also, your Searchable seems to be based
on predicates, so I'm not sure how such
a structure would work with your library.
I did not mean that Hana had this limitation, and it 
doesn't unless I'm forgetting something. I was speaking
about a theoretical possibility, but I guess I was getting
carried away.
...
...
But do 
you have an example in mind of a place where there's an *_if 
variant missing in Hana? It would almost surely be an oversight.
replace only seems to have a predicate form.
Noting that. There's some cleanup to be done in Functor if 
I adopt the systematic approach of 
    
    algorithm    -> searches with value
    algorithm_if -> searches with predicate


Thanks,
Louis