Re: [boost] [Hana] Informal review request

Subject: Re: [boost] [Hana] Informal review request
From: Louis Dionne (ldionne.2_at_[hidden])
Date: 2015-03-05 15:36:52

Steven Watanabe <watanabesj <at> gmail.com> writes:

> 
> AMDG
> 
> On 03/05/2015 12:02 PM, Louis Dionne wrote:
> > Steven Watanabe <watanabesj <at> gmail.com> writes:
> > 
> >>
> >> I've just glanced over the main page of the documentation
> >> and I had a few comments:
> >>
> >> - _tuple<Person, Car, City>
> >>   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<Types...>` 
> > 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<int, 3,2,1>;
    BOOST_HANA_CONSTEXPR_ASSERT(sort(xs) == tuple_c<int, 1, 2, 3>);

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