[boost] Variant Library Review: implementation issues

From: Gennadiy Rozental (gennadiy.rozental_at_[hidden])
Date: 2003-04-02 03:54:39

Implementation issues comments:

> > 2. is_static_visitable description states that this metafunction will
> > check whether the specified type is visitable by a static visitor.
> > This statement is misleading, cause you do not (and could not ) check
> > this. What you check is that type could be visited using
> > static_visitable_traits interface. You never mention static_visitor
> > here.
>
> The only way visitation by a static visitor can happen is if the
> specified type derives static_visitable directly OR if it appropriately
> specializes static_visitable_traits. Accordingly, what
> is_static_visitable determines is whether objects of the specified type
> derives static_visitable OR if static_visitable_traits is specialized.
>
> Please elaborate if you feel something is wrong in my approach.

In my interpretation static visitable for wide concept that allows type T to
be visited not only by static visitor but any other type. Look onto
test_static_visitable test I supplied.

> > extractable.hpp
> > 2. Extractible is the concept and should contain is_extractable
> > check, which is for some reason located in details of extract
> > header and named is_directly_extractable (this name even
> > contradicts the comment in it's definition: it's either
> > directly extractable or through custom traits implementation)
>
> If you looked more closely, you would notice that the
> is_directly_extractable is in a detail namespace. That said, I'm not
> sure what you mean when you say the name "contradicts" its comment.
> Please explain.

Here is your comment in is_directly_extractible implementation
// directly-extractable || NOT default-traits
So it's not only directly extractible but also ...
What I propose is to move it out of detail namespace and make part of
concept definition.

> > 2. From what I view in this header there are 3 ways to make type T
> > work with extract<T> interface
> >   a) Make T "directly extractible", iow inherit from extractable
> >   b) Make T "indirectly extractable" by means of specializing
> > extractable_traits
> >   c) Make T visitable by extractor_visitor, by means of specializing
> > static_visitable_traits for T
>
> Regarding a) and b), you are correct. And while your comment in c) is
> technically, your wording suggests to me you might not understand
> visitation completely. The type T you describe does not support the
> given visitor explicitly. Rather it takes visitors via templatized
> parameters.

Look into test_extract test. Here I made struct VisitableByExtractor to go
third road. It's not directly nor indirectly extractible. But still I a able
to use boost::extract interface.

> > BTW it may worth mentioning in docs. Now it appears that having
> > extract::visitor in private part of the class definition make third
> > way less usable cause I could not mention Visitor name
>
> I'm not sure what you mean here. The visitor is _not_ intended to be
> used directly.

I believe this concept deserve it's own place out of boost::variant details

> > 3. Defining result type in extract::visitor definition is unnecessary
> > and in fact prohibit compilation on MSVC6.5
>
> Please explain. I have not attempted compilation on MSVC prior to
> version 7.

    struct visitor
        : static_visitor<>
    {
    public: // typedefs

        typedef void result_type;

Why do you need this typedef? static_visitor<> will do this for you. MSVC6.5
will require you to use void_ type cause it does not support return void
semantic.

>
>
> [snip]
> > 5. Name for the visitor could be more specific. For example the one
> > you have in it's description
>
> I don't understand why it needs to be more specific. It is a private
> nested class. If you disagree, please explain why.
>

I could use this visitor to implement third way to support extract
interface.

> > define_forwarding_func.hpp
> > This file contain mix of Unix/Windows EOLs.
>
> Please explain this problem and tell me how to fix it. Thanks.

Try to print this file on Windows. Some lines disappear or became 1 line.
Run something like unix2dos for example.

> > delayed_apply_visitor.hpp
> > I did not touch binary side but once you fix the traits
> > implementation unary function operators became much more simple.
> > You don't even need PP anymore, but straightforward:
> >     template <typename Visitable>
> >     result_type operator()( Visitable& visitable )
> >     {
> >         return apply_visitor(visitor_, visitable);
> >     }
>
> Again, the code you provide does not allow temporaries to be passed to
> operator().

Right. I expected Visitable to became 'T const' for temporaries. I will
think more about it.
BTW in your version you are missing return keywords.

> > incomplete.hpp
> > This header presents yet another implementation of smart pointer
> > idiom. In this case with deep copy semantic. Do we need one? If
> > yes, lets name it deep_copy_ptr.
>
> boost::incomplete is _not_ a smart pointer. It is a value type that's
> allocates its content on the heap.
>
> So no, deep_copy_ptr is not an appropriate name.

This semantic is very close to what "real" deep copy smart pointer would
have and could easily be generated using smart_ptr framework. The only
difference is that incomplete allocate memory itself

> > 2. aligned_storage component is not that generic as it may look from
> > the perfunctory look. It only works of size is 2^N. May be we should
> > check this explicitly for more clear error notification.
>
> Please explain.

Try to define aligned_storage<5>. Do you see an error message?

>
> > 3. Why is it noncopyable??
>
> The intention is to prevent blind copying of aligned_storage since it
> is not possible to invoke the copy constructor corresponding to the
> storage's content (if any).
>
> So I ask, why *not* noncopyable?

What If I do want to make a copy and I now that all types have trivial copy
constructor?

> > visitor_ptr.hpp
> > 1. why visitor_t is not function1<>?
>
> I'm not well accustomed to the Boost.Function library, but my limited
> understanding suggests the "catch-all" behavior of visitor_ptr_t is not
> implementable.

Could you clarify above please?

> Further, I am working on a larger Boost.Visitor library. That is,
> visitor_ptr will need to return a function object that serves as both a
> static and dynamic visitor. (See the code in the sandbox for further
> explanation.)

So how this prevent you from using function<1> for implementation in
creation interface

> > * I would prefer if you would follow boost recommendation with class
> > data member naming with leading m_ instead of trailing _.
>
> Where are these recommendations?

Funny, I couldn't find it. Does anybody know where is it now?