Fix shared_vector<>

[mdavidsaver]

Fix some issues w/ shared_vector<> resulting in compile errors. Added test cases and documentation edits

1. Allow static_shared_vector_cast<>() no-op casts (eg. void -> void). Was a compile error
2. freeze(shared_vector&) failed to compile due to missing friend and typedef
3. freeze(shared_vector&) would have lost the original type due to a missing initializer. Masked by previous compile error.

[dhickin]

Looks good. The changes do indeed fix the static cast no-op issues and the test cases and compile-time error checks look great.

I did wonder about the use of meta-programming in selecting the cast though. I can see it makes the code shorter to use the meta programming checks for void-ness and same constness, but it also makes it slightly harder to see what's going on. A slightly more brute force approach where the specialisations for each version of const-ness, voidness would be more transparent at the cost of being only very slightly longer. It's a trade off, but given the code for the cast is one line the extra code isn't too painful.

I wrote a quick implementation of the brute force approach - see what you think. I'll add in separate comment.

Just my opinion - shared_vector's your code so you decide.

Couple of (very minor) things

1. Do you need to provide the default implementation of static_shared_vector_caster
   struct static_shared_vector_caster { /* no default */ };
   Would
   struct static_shared_vector_caster;
   be sufficient?
2. Could we change the variable name of the const version of "untyped", just in case it sets of an alarm somewhere? Unlikely to cause a problem I know, but safer to change it.

[dhickin]

I think this should accomplish the same thing without the meta-programming and is about 24 lines longer. (It passes the run-time checks and the compile fail checks for me).

namespace detail { template<typename TO, typename FROM> struct static_shared_vector_caster;

// from void to non-void with same const-ness
template<typename TO>
struct static_shared_vector_caster<TO, void> {
    static inline shared_vector<TO> op(const shared_vector<void>& src) {
        return shared_vector<TO>(src, detail::_shared_vector_cast_tag());
    }
};

template<typename TO>
struct static_shared_vector_caster<const TO, const void> {
    static inline shared_vector<const TO> op(const shared_vector<const void>& src)    {
        return shared_vector<const TO>(src, detail::_shared_vector_cast_tag());
    }
};

// from non-void to void with same const-ness
template<typename FROM>
struct static_shared_vector_caster<void, FROM> {
    static FORCE_INLINE shared_vector<void> op(const shared_vector<FROM>& src) {
        return shared_vector<void>(src, detail::_shared_vector_cast_tag());
    }
};

template<typename FROM>
struct static_shared_vector_caster<const void, const FROM> {
    static FORCE_INLINE shared_vector<const void> op(const shared_vector<const FROM>& src)    {
        return shared_vector<const void>(src, detail::_shared_vector_cast_tag());
    }
};

// cast to same type, no-op
template<typename TOFRO>
struct static_shared_vector_caster<TOFRO,TOFRO> {
    static FORCE_INLINE const shared_vector<TOFRO>& op(const shared_vector<TOFRO>& src) {
        return src;
    }
};

template<>
struct static_shared_vector_caster<const void, const void> {
    static FORCE_INLINE const shared_vector<const void>& op(const shared_vector<const void>& src) {
        return src;
    }
};

template<>
struct static_shared_vector_caster<void, void> {
    static FORCE_INLINE const shared_vector<void>& op(const shared_vector<void>& src) {
        return src;
    }
};

// const to non-const conversion disallowed
template<typename FROM>
struct static_shared_vector_caster<void, const FROM> {};

// const to non-const conversion disallowed
template<>
struct static_shared_vector_caster<void, const void> {};

// non-const to const conversion disallowed
template<typename TO>
struct static_shared_vector_caster<const TO, void> {};

template<>
struct static_shared_vector_caster<const void, void> {};

} // namespace detail

[mdavidsaver]

Do you need to provide the default implementation of static_shared_vector_caster

Not providing a default implementation changes the error gcc gives from "op is not a member of" to "incomplete type", which imo. makes little difference (both equally opaque to the uninitiated). I slightly prefer having an empty default definition inline when reading the code since seeing it tells me that there can be no definition later, so I don't have to go looking for one.

Also, I have some idea at some point to explore using attribute((error("cast not supported"))) in the default definition to provide some actually meaningful error message, at least with gcc.

[mdavidsaver]

Could we change the variable name of the const version of "untyped", just in case it sets of an alarm somewhere? Unlikely to cause a problem I know, but safer to change it.

A conflict here seems doubtful to me (untyped ~= void). "untyped" isn't a reserved word in any published or draft version of the c++ standard. Trying to predict what may be added in future seems a losing proposition (#include "crystal_ball.h").

http://en.cppreference.com/w/cpp/keyword

[dhickin]

A conflict here seems doubtful to me (untyped ~= void) I think you may have missed the point. I was suggesting choosing a name that doesn't start with an expletive. As I said: I'm pretty sure it would never cause a problem, but safer to change it.

Re point 1) fair enough. Like I say, it's your class.

[anjohnson]

Michael might not even realize that there is a 4-letter expletive at the start of the name c*ntyped since the term is not really used over on this side of the pond. I would second David's suggestion that you consider changing it though, it might get caught in UK mail filters.

[mdavidsaver]

I think this should accomplish the same thing without the meta-programming and is about 24 lines longer. (It passes the run-time checks and the compile fail checks for me).

As far as I can tell your example should work, though it may have some redundancy (eg. <void,void> should be captured by <TOFRO,TOFRO>)

IMO this is a case of finding the lesser of two evils: enablers vs. specializations.

Personally, I prefer enablers in this case as:

• The statement of the logic is more direct, and doesn't by itself require knowledge of the template specialization selection rules.

typename meta::_and<meta::same_const<void,FROM>, meta::is_not_void<FROM>::type

Which I think reads fairly easily to someone who has seen prefix expression notation as same_const<void,FROM> && is_not_void<FROM>.

I'll grant that, for a beginner, figuring out how this is implemented is quite confusing. However, IMO the selection rules are no better.

• With enablers, logic errors are more likely to result in compilation errors. I would like to minimize the possibility of allowing bad casts/conversions to compile.

In your example you combine positive and negative specializations, which I would not do. <TO, void> would by itself match <const int,void> which would bypass freeze(). However, in your example this should be matched by the negative <const TO, void> which is more specific. However, if <const TO, void> were removed (eg. a mis-merge), or if we misunderstand the selection rules, or a compiler doesn't correctly implement them correctly, the result would be unexpected success.

Actually the present code is already compromising on the use of enablers. The four conditions could be combined into two with the addition of a third enabler condition.

typename meta::_and< meta::_and< meta::is_not_void<TO>, meta::is_void<FROM> >,
                                     meta::same_const<TO,FROM> >::type>

which is to say is_not_void<TO> && is_void<FROM> && same_const<TO,FROM>.

The driving requirement for cleverness here is to avoid explicit specializations for all the non-void types. So this use of is_void<T> seemed like not enough reduction is source length to offset the potential confusion of a second _and.

[mdavidsaver]

Michael might not even realize that there is a 4-letter expletive ...

Nope. This went right past me. ... now that I've stopped laughing, sure I'll change this.

[dhickin]

As far as I can tell your example should work, though it may have some redundancy (eg. <void,void> should be captured by <TOFRO,TOFRO>) <void, void> should always match to the <void,void> specialisation which is a better match than the partial specialisation <TOFRO,TOFRO>. Without the <void,void> specialisation <void,void> would be ambiguous as it would match <void,TOFRO>, <TOFRO,void> and <TOFRO,TOFRO>.

The choice of enablers versus partial specialisations is probably just a matter of taste. Happy to go with your preference.