Re: [boost] Boost.Conversion review

Subject: Re: [boost] Boost.Conversion review
From: Vicente J. Botet Escriba (vicente.botet_at_[hidden])
Date: 2011-08-16 18:31:09

Le 16/08/11 23:17, John Bytheway a écrit :
> This is not a review.  Just some comments on the docs.  I have other
> issues with the design which I'll raise in another post.
Hi,
thanks in advance for your useful and constructive comments.
> The one long page of docs is a bit intimidating.  At least it should be
> split into reference and non-reference, I think.
I could change that. I guess that then there will be a page for each 
first level section, which is admisible. I will do it if this is 
considered a better choice.
>
> Overview
> --------
>
> - s/Tatget/Target/
OK. Done.
>
> - In this:
>
> template<  class Source>
> convertible_from<From>  implicitly(Source source)
> {
>    return convertible_from<Source>(source);
> }
>
> I guess you mean "convertible_from<Source>" as the return type?
Yes. Done.
> - When first discussing the need for is_constructible and is_assignable
> (under "Caveats"), could you mention that there is a list of supported
> compilers just below (to avoid scaring away people who don't know
> whether their compiler is supported)?
Good idea. The fact that I didn't reached to implement these traits on 
some compilers was quite frustrating. For the moment I have reached to 
implement it for gcc >= 4.4 and clang. Even if MSVC supports decltype I 
don't reach to make it work.
> User's Guide
> ------------
>
> - You say it needs "compilers supporting decltype" -- would typeof
> suffice also?
At the end I think that what is needed is just SFINAE_EXPR. I'll try 
TYPEOF and see how this behaves.
>
> - "When you need to make a extrinsic explicit conversion" is ambiguous;
> I suggest "When you need to perform an extrinsic explicit conversion".
OK.
> - In "they can try with the make assigner to `lvalue' free function" you
> have bold 'm', 'a', 't'; is this because the function used to be called
> "mat"?  If so, they should probably be un-bolded.
Yes. This was the preceding name. I have removed this now.
> - "Why ADLIt would be great if" looks like a mistake.
Done.
>
> - In this:
>
> template<typename T>
> typename enable_if<is_extrinsically_convertible<int>,void>::type
> f(T v) { return convert_to<int>(v); }
>
> surely you need to pass T to is_extrinsically_convertible?
Yes. Done.
> - Does is_extrinsically_convertible test for explicit or implicit
> convertibility?  Is there a trait for the other one?
It test for implicit conversion. There is a trait 
is_extrinsically_explicit_convertible.
> - I presume s/implicit_convert_cp/implicit_converter_cp/
Done.
>
> - What does the "cp" signify?  I figured it out eventually but you
> probably want to say.
_cp stands for customization point. I will add a more specific explanation.
>
> - I assume there is also explicit_converter_cp and an
> implicit_convert_to function you can overload with dummy_type?  You
> should probably at least mention them in passing when introducing
> implicit_converter_cp and explicit_convert_to.
OK. I will do it before the review.
>
> Reference
> ---------
>
> - The docs of BOOST_CONVERSION_NO_IS_ASSIGNABLE say "Macro stating if
> the compiler doesn't support...".  Please clarify in what way the macro
> "states" this; I think you mean "Macro defined if the compiler doesn't
> support..." (and personally I would prefer "Macro defined if and only if
> the compiler doesn't support...").  The other macros similarly.
OK. I will do it before the review.
> - Please normalise the names of
> BOOST_CONVERSION_NO_IS_EXPLICIT_CONVERTIBLE and
> is_explicitly_convertible; the former should have the "LY".  Similarly
> BOOST_CONVERSION_NO_IS_EXTRINSIC_ASSIGNABLE,
> BOOST_CONVERSION_NO_IS_EXTRINSIC_CONVERTIBLE.
OK. I will do it before the review.
> - You list all these headers providing specializations for standard
> types, such as "Type traits specializations for<string>  types." but
> it's far from clear what these mean.  There are more details lower down,
> but you wouldn't know that from looking at the short version.  Can you
> point or link to the further details?
OK. I will try to do it before the review.
> - "implicit conversion of the @ Source" -- I guess the @ is a typo?
Yes. It should be @c.
>
> - "Defines the free function assignable_to class" -- is it a function or
> a class?
It is a class. Corrected.
>
> - ExtrinsicallyExplicitConvertible should probably be
> ExtrinsicallyExplicitlyConvertible for consistency.
I was wondering how tis should be. If you can confirm that this is the 
correct way I will change them.
>
> - boost/conversion/convertible_from.hpp is documented as "Defines the
> free function implicitly.".  I have two problems with this: Surely it's
> more significant that it defines convertible_from?  Also, I think
> implicitly warrants its own header; I'm unlikely to remember that this
> is what one must include to get it.
OK I understand. I will add a implicitly.hpp and a lvalue.hpp headers.
>
> - What are the semantics for the conversions to/from std::string?
Currently the library uses lexical cast and I know that this is a 
controversial issue. I will add it in a more explicit way.
>
> - Do the fusion::tuple conversions really only go up to 3-tuples?  What
> about 1-tuples?
I have no taken the time to implement a preprocessor version. I will do 
it if the library is accepted.
> - Must all the "trick_*" classes really appear in the docs?
This is the first time I have used doxygen to generate the docs. I have 
find a lot of trouble to ensure that the comments in the source appear 
completly in the doc, and I know that there are some cases that are 
missing. Most od the time, this are bugs associated to how boost book 
takes in account the xml files generated by doxygen, maybe be there are 
some cases associated to doxygen itself, I don't remember. Unfortunately 
if I remove these classes, other specializations will disappear from the 
documentation. I will try to identify a simple case causing the error 
and make a ticket.
> John Bytheway
Thanks again,
Vicente