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.
