[boost] [Review] Boost.Convert, review manager analysis

Subject: [boost] [Review] Boost.Convert, review manager analysis
From: Edward Diener (eldiener_at_[hidden])
Date: 2011-05-06 11:01:06

As previously stated Vladimir Batov decided to withdraw his library at 
the end of the review period from consideration for inclusion into Boost 
and I had announced this and the fact that because of this the library 
would not be accepted.

Nonetheless I want to give my analysis of the reviews and the issues 
both pro and con of the reviewers. I also want to give some of my own 
thinking on these issues.

There were few reviews of the library but much late discussion in the 
final day of the review period and just afterward.

There were 2 reviews which voted to accept Convert ( Artyom Bellis, 
Alfredo Correa ), 3 reviews which voted not to accept Convert ( Jeroen 
Habraken, Thomas Heller, Hartmut Kaiser ), 1 review which would have 
accepted Convert but qualified that only if lexical_cast could not be 
extended instead ( Rob Stewart ), 1 review which would have accepted 
Convert with large scale changes ( Gordon Woodhull ), and one mention, 
not a formal review, of acceptance until something else came along but 
with perhaps a later change of mind ( Paul Bristow ). There was also one 
review sent to me personally by Barend Gehrels, after Vladimir Batov 
withdrew his library, which voted not to accept Convert. There were also 
a number of people who raised doubts and objections to Convert in 
various ways without giving a formal review.

It is all these issues raised by reviewers and commenters I want to 
discuss next. Vladimir Batov was responsive to nearly all of those 
issues but I want to enumerate them and give my own thoughts about them 
as I read reviews and comments.

A number of people wanted lexical_cast to be extended instead of a new 
library. This certainly is a valid viewpoint. But I believe that the 
lexical_cast library is set as it is until someone takes it over from 
its original author and is willing to change it. I think it is better to 
review the library under discussion rather than to hope that something 
else can be changed in the future.

There was an issue of the basic syntax which many people did not like. 
Instead of the syntax of 'convert<dest>::from(source)' there were a 
number of people who wanted to see 'convert<dest>(source)' instead, 
largely becaused it matched the 'lexical_cast<dest>(source)' syntax. 
Vladimir Batov explained why he used the syntax he did. I think in this 
case it is reasonable. Again this is an issue in relation to 
lexical_cast, and I saw no real significant difference between the 
syntaxes on a basis of practical use.

The main issue brought out by many people is whether a single function 
can encompass successfully all of the functionality which Convert wished 
to encompass and whether this functionality both worked smoothly with 
everything others wished to do with the functionality. Here there were 
many valid objections and I will attempt to enumerate them:

1) The return value of the convert function is really many different 
returns, and this is done through implicit conversions of the actual 
return type. This was mentioned most eloquently by Thomas Heller. It is 
quite natural to assume that the return value of a successful call to 
'convert<dest>::from(source)' would be a 'dest' object but instead it is 
a value which has an implicit conversion to 'dest'. This works when 
passing the value to ordinary functions but does not when passing the 
value to template functions. As Vladimir Batov mentioned one can use the 
'value()' member function of the return value, and presumably also one 
can use a 'static_cast<dest>' also, to force the exact 'dest' object to 
be passed, but I also viewed this as a negative way to work, and a very 
valid objection to the way Convert was designed. To me it was not just a 
matter of confusion to the end-user but of a naturalness of use. A 
convert-type interface should have some function where, if the convert 
is successful, the actual result of the conversion is returned, and not 
some intermediate value with an implicit conversion.

2) The Convert library uses a stream-based method to do conversions but 
wanted to leave the door open for other methods. Part of the extended 
syntax of the library works with stream manipulators and locales. A very 
valid point was brought up, again by Thomas Heller, that it is not clear 
how the extended syntax is supposed to work with and affect non-stream 
based specializations of the basic convert template. Even the method of 
extending the library in this way, rather than just providing 
stream-based operators for a given type, is open to much doubt as valid 
C++ and largely left unexplained in the documentation.

3) The syntax for the stream-based and locale additions to the library's 
functionality was found odd by many people. The term 'wacky' was often 
used and Vladimir Batov did defend his choice about it well.
Nonetheless it is a valid issue to consider whether the extended syntax 
is easy for a person to use. I found the syntax for using iostream 
manipulators quite natural, and the syntax for locales and other 
functionality such as throwing behavior less natural. Again I do not 
want to get too caught up in syntactical issues but I did understand the 
issues brought up by others regarding this.

4) The method of using the convert function in algorithms was 
criticized. Some people expressed their wish that a placeholder syntax, 
as used by other Boost libraries, be used. I actually found the way 
Convert does this as pretty elegant, there were two syntaxes which 
worked, but could understand others liking for the placeholder syntax.

5) The use of a default value, the issue of a default constructor, and 
the differing results when a conversion fails or not, received much 
discussion, especially at the official end of the review period and 
afterward, and I appreciate reading all of the discussions. I do not 
want to specifically go into each one, other than to say that I think 
this has basically to do with the issue of having a single function do 
everything which could not easily cover all the different scenarios 
mentioned for actual use.

There were other valid concerns which are related to just a need for 
more complete documentation. Vladimir Batov responded to most of them. 
Here I will enumerate them:

1) Rob Stewart in his review mentioned many documentation corrections 
related to grammar, spelling etc.
2) What locale is being used by default is not explained.
3) The reference is very incomplete.
4) The section for extending the library is unclear and incomplete.

Finally there were valid concerns relating to getting Convert to work 
properly. Some of these related Convert and lexical_cast and noted the 
differences. A few others mentioned things which just did not work 
properly in the actual conversion and again Vladimir Batov said he would 
look at these issues. Barend Gehrels sent me a late review, which he did 
not want to post to the mailing list because Vladimir Batov had already 
withdrawn is library, in which he said that he had some technical 
problems adapting a Boost.Geometry type to work with Convert. I think 
these are all technical issues which can be resolved.

Many other people had specific incisive remarks during the review period 
but I can not list them all. That does not mean those remarks were 
ignored by me in any way, as I read and attempted to digest what 
everyone had to say.

Final remarks:

I would like to bring some clarity to the issues which may have forced 
Vladimir Batov to essentially withdraw his library at the end of the 
review period from consideration and which led to much comment and 
criticism about the library. This is my purely personal take on the 
library, but it is based on my technical understanding of the issues 
involved.

The main issue with the library is that the 'single function fits all 
scenarios' works much less well in practical use than the author 
supposed, even as I understood the reason for simplicity which caused 
him to pursue this path.

There really needs to be a function that simple returns the converted 
value upon success. Overloaded versions of that function, or differently 
named functions, can return other specific things. Where there is a 
great attraction to a return value which can be implicitly converted to 
other values, I think this attraction given practical use is somewhat 
illusory.

While I did not enjoy some of the harsher remarks made by Thomas Heller 
in his review of the library about the design of Convert ( and I do not 
think he really meant them as harshly as they may have sounded ) he did 
rightly point out that the 'overloaded' return value is an impediment to 
a cleaner design which would allow one to use the right convert function 
given the possibilities which the library wants to accomplish.

I know there has been many subsequent discussions about a set of 
functions that can replace the single function in Convert depending on 
how Convert can be used, and I do not myself want to get in the middle 
of this debate, but once there is an understanding of what needs to be 
done, I really think the library can be designed to appeal to end-users 
in a better way.

The second issue in my mind is that the library attempts to do too much 
given the way it was designed. If the focus is simply a library which 
does stream-based conversion between strings and objects ( both ways ) I 
personally think it would be much better. Once you look for non-stream 
based conversions you have to consider more carefully how the 
stream-based syntax and locales should be integrated with the library. 
Once you look for type to type conversions rather than string to type 
and type to string conversions you have to design a good general 
facility for specifying how this is done within the functions you 
provide. This is a much bigger job of thinking about design, unless you 
are simply providing a very generalized shell with little real 
functionality built-in. I am not saying the author of the library has 
not thought about these issues, but my impression is that they have not 
been addressed carefully.

Just in case anyone wanted to know, and also so that Vladimir Batov does 
not feel so bad about withdrawing the library from consideration at the 
end of the review period, I would have rejected the library for 
inclusion into Boost given the reviews and comments of others and my own 
perceptions of the issues involved. But I think the author has still 
done a fine job and is close to having a library which could be included 
as a part of Boost.

Although it is hard work, it has been a pleasure to be a review manager 
of this library and I thank everyone involved and especially the author 
of the Convert library himself.

Edward Diener