Re: [boost] [Hana] Informal review request

Subject: Re: [boost] [Hana] Informal review request
From: Matt Calabrese (rivorus_at_[hidden])
Date: 2015-03-05 15:15:33

On Thu, Mar 5, 2015 at 6:41 AM, Louis Dionne <ldionne.2_at_[hidden]> wrote:

> The library is available at [1] and the documentation at [2]. Comments
> should
> be directed to this mailing list, while bugs and other similar problems
> should
> be directed to the GitHub issue tracker. Please participate, especially if
> you have interest and/or experience in metaprogramming; after all, you
> represent the target audience for Hana.
>
>
> Thank you,
> Louis Dionne
>
> [1]: https://github.com/ldionne/hana
> [2]: http://ldionne.github.io/hana/

Looking great. I do have one (imo, very big) issue though, but one that can
be resolved. I notice you are still doing things like:

static_assert(pointers == tuple_t<Car*, City*, void*>, "");

I think I brought up the subtle problem with this a CppNow last year.
Because == is used, which is an unqualified function call, it means that
ADL takes place. Because ADL takes place, it means that all associated
namespaces need to be considered. Because tuple_t<...> yields an instance
of _tuple<...>, if I understand correctly from your current description,
included in those associated namespaces are things like all of the base
classes of template arguments of the types involved (in this case, any base
classes of Car and City). What this implies is that simply by writing
pointer == tuple_t<Car*,...>, the effect is that the type "Car" is
implicitly instantiated at this point (if it were template instantiation),
because the compiler needs to see if there are base classes in order to
properly form the set of associated namespaces!

In this particular example it might not be too apparent why this is so bad,
but in the general case, instantiating such types that are involved can
cause a number of serious issues:
1) Perhaps the most obvious is that instantiation can potentially fail (and
therefore cause a compile error even though it doesn't look like the type
would be used).
2) They can cause considerable compile-time overhead for a template
definition's instantiation that otherwise might not be used.
3) Worst case, but probably the least likely. It can compile but produce
strange results in the program in a very subtle way, not even necessarily
directly at this point in code, due to the type being instantiated
"prematurely" with respect to what the user may have expected. This can can
cause problems, for instance, if certain types are still incomplete at this
point in the translation unit, or if important functions/overloads have not
yet been declared, etc. Because of memoization, at the point that the user
thinks they are first instantiating the template, they will get an
instantiation that they likely did not expect.

As great as it is to be able to use operators, I don't think it should be
recommended (or probably supported, unless these issues can be fixed). As I
see it right now, ADL here should *always* be avoided, whether it's an
operator or not, for similar reasons. Instead, it probably makes sense to
have all "functions" be global function objects and/or variable templates
so that people can't accidentally use ADL. Similarly, that point needs to
be stressed for users who wish to create their own metafunctions. This is
an unfortunate subtlety that would be very much important for
metaprogrammers to understand so that they too wouldn't accidentally fall
victim to them.

An alternative to this kind of fix that might allow operators to still be
used is to make the tuple type not (directly) be a template instantiation
with template arguments by instead (as an example) making the tuple type a
non-template local type that is declared inside of a function template.
You'd return an instance of this type from the function, and use a c++14
auto-deduced return type. The operator would be defined in that function
template's namespace or in another associated namespace of that local type.
The downside of this approach is that (I believe) it removes any ability to
have something like a namespace-scope _tuple<T, U> type/alias be used in a
function parameter list in the case that T and U are to be deduced, unless
there is some other trick that can be thought of.

I'm sure these problems can be resolved, but I definitely think it is
important to fully address them before formal review. The subtlety of the
problems and the ease of misuse makes things really scary to me and I would
be worried if a lot of library code ended up getting written on top of this
without solutions in place.

If any of this is already solved by way of tricks that I'm not aware of (I
haven't looked through the source yet), then just make sure people know
that they, too, must utilitze such tricks when making their own hana-style
metafunctions.

-- 
-Matt Calabrese