[Boost-users] Convert library review manager result

Subject: [Boost-users] Convert library review manager result
From: Edward Diener (eldiener_at_[hidden])
Date: 2014-06-01 18:02:43

This is my analysis of the 'convert' library review and the result
of whether the convert library should be accepted into Boost.
Please read the entire analysis to understand my reasoning before
reading the result.

I would like to thank all of those who took part in the review and made
comments. This includes Julian Gonggrip, Rob Stewart, Andrzej Krzemienski,
Matus Chochlik, Jeroen Habraken, Erik Nelson, Hartmut Kaiser, Joel De 
Guzman,
Thijs (M.A.) van den Berg, Roland Bock, Gavin Lambert, feverzsj, Paul 
Bristow,
and alex. If I have missed anybody I do apologize. All comments were taken
into account and all comments were intelligent and to the point.

Naturally I would also like to than Vladimir Batov for patiently answering
all comments and explaining and defending his library and his design 
decisions.

For the most part I am going to focus on general issues rather than on the
individuals who made them.

Documentation issues:

The documentation was too focused on explaining the
library as it differed from lexical_cast. Vladimir Batov explained why
this is so but I also agree that the documentation can be better by simply
focusing on what the library provides in terms of functionality. Comments
also mentioned that the current converters are not documented as well as
the could be, and some commenters reflected that knowledge of how a 
particular
converter works, including possible failures using that converter,
is also needed despite the common library interface.

The documentation did not provide enough information
about the use of locales in the library.

The reference section was very limited with not enough explanation. A 
reference
built from doxygen comments in the source files would be better.

A better explanation or example in the documentation for writing one's own
converter was suggested.

Although the Performance section in the documentation was fairly lengthy
some commenters wanted to know why some things could not be tested. 
Others wanted
more information about how convert performance compared to other 
convert-like
implementations.

In general I think most reviewers found the documentation adequate for
understanding how to use the library but lacking in completeness. Vladimir
Batov reflected that if the library were accepted into Boost he would then
create more complete documentation and the above issues could be addressed.

Implementation issues:

As in most reviews most commenters did not look very deeply into the actual
code but were happy to know how to use the library, and this is 
understandable
for a review. There were a few pertinent comments however and I am just 
going
to enumerate them.

All macros should be prefix with BOOST_. My own addition to that is that all
macros should be prefixed with BOOST_CONVERT_ to avoid any conflicts 
with other
libraries.

There should be a jamfile for running the tests. Also some full-blown 
examples
of using the library in a simple program, along with the appropriate 
jamfile,
would be helpful.

Source code should contain spaces, not tabs. This is an easily fixed 
minor issue.

As mentioned before doxygen comments in the public source are the usual 
Boost
way of doing things.

The main header file should be convert.hpp instead of api.h.

Some includes were missing from the printf converter.

Design issues:

In the design of the library is where we have the most varying views. 
The major
issue in discussing the design of the library for those who felt it was 
inadequate
were the comments that using the library was too complicated, that the 
central
function for invoking a conversion should be simpler to use. While all 
suggestions
were well-argued I felt that a number of these differences were merely 
syntactical
issues rather than issues that made the functionality easier to use. The 
main
interface for using the library is:

   convert<output_type>::from(input_value, converter)

Vladimir Batov had asserted during the review that this could be 
simplified further
by removing the need for the 'from::' part of the signature which would 
make the syntax
simpler. Some reviewers did not like the name 'convert' and offered 
substitutions, others
wanted the default value, currently specified as '.value_or(default)', 
to be specified as a direct
input parameter, others wanted the converter itself which is an input 
parameter to be specified
via another syntax. All of these are too me syntactical issues and no 
resolution could
possibly please everybody but it is understandable that different 
reviewers have different
syntactical preferences.

More to the point were some changes in actual functionality. The three 
most important
involved the return value, type requirements for the output type, and 
the use of a default
converter. Considering each in its turn:

1) The return value is some form of 'optional', with the idea of using 
boost::optional
if it meets all the requirements needed by the 'convert' library. A 
number of people
wanted the actual value returned without having to append .value() to 
the convert signature to get it.
They were also concerned with the performance overhead of having to 
create an optional
value as the immediate return value. It was also suggested that while 
the current
convert syntax might be fine, some alternatives directly returning the 
value, for those
who like maximum simplicity, might be possible.

2) The type requirements for the output type are default constructible 
and copy
constructible. A number of reviewers felt that if a non-const reference 
were passed
to the convert function as an output instance then the type requirements 
would not have
to be met. However the type requirements of some of the underlying 
converters might be
affected by this and the documentation does explain how to work around 
the default
constructible requirement, but I still do understand the reasoning 
behind this possibility.

3) A number of reviewers wanted a default converter to be used but there 
was hardly an
agreement on what this default converter should be, some favoring 
lexical_cast, others
favoring stringstream, and no doubt others might want something else. 
Nevertheless
providing a default constructor interface, possible with a macro defined 
a compile time,
seems like it might be doable.

There was further discussion about the actual callable signature for a 
plug-in converter.
Here there was less contention about the interface, which is currently a 
callable with a
signature of:

   template<typename TypeOut, typename TypeIn>
   bool operator()(TypeIn const& value_in, TypeOut& result_out) const;

Vladimir Batov did suggest some possible alternatives and there was a 
discussion
with a number of reviewers about alternatives that seem implementable. 
This also ties
into the concern of some reviewers that it should be straightforward to 
create one's
own converters with minimum restrictions on the input and output types, 
as well as
minimum performance overhead.

Votes:

A number of reviewers made comments but offered no final vote on whether 
or not the
conversion library should be accepted into Boost. Some reviewers offered 
a Yes vote, but
heavily based on changes they want to see. I will attempt to tabulate 
them here:

YES votes:

Christopher Kormanyos
Paul Bristow
Julian Gonggrip ( with some reservations which Vladimir Batove has met 
or may easily meet )
Andrzej Krzemienski
Matus Chochlik

YES, conditionally but otherwise NO if conditions are not met, which the 
current version does not meet]

Jeroen Habraken
Roland Bock
alex

NO

Rob Stewart

Final Decision:

Among those who voted YES conditionally but otherwise NO there was a 
general consensus
including Vladimir Batov, the 'convert' library creator, and some of the 
others that an attempt
could be made to satisfy some of the conditions for simplification 
and/or functionality they
desired.

My decision, based on the votes as I interpret them, is that the convert 
library is ACCEPTED
into Boost. While a perfect design is impossible I believe that the 
design of the library
is an adequate starting point for a general-purpose conversion framework 
and that the design
could change incrementally enough to satisfy some of the qualms of those 
who might not find
it as initially useful as they have hoped, while still maintaining the 
central framework
which Vladimir Batov has created. Furthermore I believe that others have 
reflected that a
general-purpose conversion library would be very useful if it were 
flexible enough to support
plug-in converters created by programmers, as the 'convert' library 
does. I realize that what is
essentially a 5 to 4 vote in favor of the library is not an overwhelming 
consensus by any means
but I feel that 3 of those 4 NO votes are predicated on wishes for 
possible changes to the 'convert' library
which may yet be met before the library gets officially added to an 
upcoming Boost release.

Since I have accepted the library based on the votes and my 
interpretation of the issues I
would like to encourage those who feel that some alternate designs could 
be implemented as part
of the 'convert' library to work with Vladimir Batov, if he is 
agreeable, to add those alternatives
to the library. Nonetheless it is his library and he would be the final 
arbiter of any decisions
reached. I would like to encourage the view, however, that the current 
design is what has been
voted in by others as the 'convert' library so that any changes made to 
it as it is added to
Boost should be incremental additions which minimally change what others 
see as an acceptable library.