Re: [boost] [convert] Boost.Convert Library Review

Subject: Re: [boost] [convert] Boost.Convert Library Review
From: Stewart, Robert (Robert.Stewart_at_[hidden])
Date: 2011-05-02 10:58:41

Edward Diener wrote:
>
> What is your evaluation of the design?

Easy to use, powerful, and flexible.  While the current implementation doesn't offer much optimization, the interface doesn't preclude doing so, except that
there is no way to select the backend for the conversions.  Stream-based conversions are slow, but flexible and extensible.  Other sorts of conversions are useful, too.  For example, while forgoing locale and manipulator support, strtol() or boost::spirit::qi::int_ are much more efficient.  It would be good to have a means to access such conversions when applicable, falling back on streams-based conversions when necessary.  (Perhaps this could be added as some interface in a separate namespace that falls back on the current interfaces, but some thought in this direction is warranted before the current design is cast in stone.)

One thing lacking is an interface allowing the compiler to deduce the target type:

   int i;
   convert(i, str);

or:

   int i;
   convert(i)::from(str);

Such an interface can be built atop what is now provided, of course.

> What is your evaluation of the implementation?

No.

> What is your evaluation of the documentation?

[Detailed comments on the documentation follow my answers to the standard questions.]

The documentation covers most of the features of the library, but can be made more complete and clearer.

As noted by others, the reference section needs work, including information on exceptions and points of customization that influence particular functions.  Inclusion of headers that have not value to a user of the library is questionable.  For example, the linke <boost/convert/boost_parameter_ext.hpp> is nearly useless except as a means to view the header from a browser.

There should be example code for each point of customization in the library so that users understand exactly how the library can be extended.

> What is your evaluation of the potential usefulness of
> the library?

The library has the potential to be highly useful.  The interface isn't as straightforward as that of lexical_cast, but is more powerful and flexible.  Thus, it may not quite supplant lexical_cast, but it may well become the standard tool of many rather than sometimes using one or the other.

> Did you try to use the library?

No.

> How much effort did you put into your evaluation? A
> glance? A quick reading? In-depth study?

A detailed reading of the documentation.  Past involvement in discussions on the design and interface of what became this library.

> Are you knowledgeable about the problem domain?

With conversions and IOStreams, yes.

> Do you think the library should be accepted as a Boost
> library?

I have mixed feelings on this.  If lexical_cast can be extended, then I think most of the features of this library should be made part of lexical_cast, but that would preclude the optimized conversions I suggested earlier (when one can forgo locales and manipulators).  Thus far, history has indicated that such changes are not welcome, making a library like Boost.Convert necessary.

That answer is a bit ambiguous, so let me try to be clearer.  If lexical_cast can be updated to address most, if not all of what Boost.Convert offers, then Boost.Convert should not be accepted.  In that case, a distinct library that offers a non-stream-based means to do conversions that improves performance might be desirable.  However, if lexical_cast won't be updated to include what Boost.Convert offers, then yes, Boost.Convert should be accepted, with due attention to suggestions and concerns raised during the review.

I'm concerned with the small number of reviews to date.  With so few reviews, one wonders whether the library should be accepted if there is so little interest.  It may be that many are busy with BoostCon preparations or other intrusions of real life, but the lack of response is disconcerting.

Documentation Comments
__________________________________

You'll find many suggestions in the following that are stylistic, but they are motivated by a desire for improved clarity and correctness.  Therefore, while my wording need not be retained, changes are needed in each case.

s/extendable/extensible/ for the right meaning.
s/straight-forward/straightforward/

IMO, "i.e." should always be "i.e.," because "that is" is always set off by a comma when it introduces a clause.

There's no consistency in terminology for the source and destination types.  For example, in some cases, the destination type is called the "target" type, while in others, it is the "result" type.  In other cases, they are referenced by, apparently, the template parameter names "TypeIn" and "TypeOut."  This is confusing.

Code examples should use const more.  For example, "int i = convert..." should be "int const i = convert..." in most (all?) cases.

Code examples should always compile, even if they don't do much.  For example,

   if (res) conversion succeeded

should be something like the following:

   if (res) conversion_succeeded();

or this:

   if (res) /* conversion succeeded */;

__________
Introduction

The introduction, which is the first thing people will read, needs some work.  Try this instead:

Boost.Convert builds on the ideas of the venerable boost::lexical_cast (see lexical_cast's documentation) while offering simple, familiar conversios, using a minimal interface, while extending lexical_cast's feature set with:

 - Optional, non-throwing behavior on conversion failure;
 - Support for default/fallback values to return on conversion
   failure;
 - Extra control over conversion failure checking;
 - Support for Standard IOStreams manipulator-based formatting
   (std::hex, std::scientific, etc.);
 - Locale support;
 - Support for Boost.Range-compliant char- and wchar_t-based input ranges
 - Relaxed type requirements for the target/destination type:
   DefaultConstructible support is not required
 - Exstensibility

Note: Boost.Convert does not aim to be a parser generator library. For serious parsing, consider Boost.Spirit.

__________
Motivation

The focus of this section is unusual.  Normally, the Motivation section is intended to provide usage scenarios that potential users can identify with while highlighting the library's ability to help.  Your focus is on why you created the library.  That is, you described your own use case that led to creating the library.  Put another way, what you've labeled "Motivation" is really "History" and should appear near the end of the Table of Contents.  (Obviously, this history will have to be changed if this library and the Conversion library are accepted as Boost libraries.)

Your specific use case, converting binary application data into string form (and back again) for XML to use in portable interprocess or inter-application communication, is fine, but must be developed further.  Having done that, you need to motivate use of the library in some other cases: string-to-UDT, number-to-number, etc. conversions, all without making this section too long.

In the last sentence, s/via Boost.Convert interface/via the Boost.Convert interface/.

__________
Getting Started

s/deployed/used/

s/the first deployment/the first form/

s/identical and as a drop-in/like a drop-in/

s/The second interface takes/The interface used in the second form requires/

s|returned if/when|returned if|

s/That might be quite a deal-breaker...constructors/Forcing the DefaultConstructible requirement on target types prevents using boost::lexical_cast on some user-defined types./

s/variety of conversion deployments/variety of conversion use cases/

s/For example, an application/For example, consider an application that/

s/application needs to stay operational and to maintain its internal integrity despite the not-too-remote possibility of/application must work despite/

s/Like the following/The following code illustrates how this might be done:/

I suggest the fallback_1/2 example code should be structured like this:

   type1 p1 = convert...;
   if (p1 == fallback_1) ...;
   type2 p2 = convert...;
   if (p2 == fallback_2) ...;

s/the deployment of boost::convert/using boost::convert/

s/boost::lexical_cast deployment/using boost::lexical_cast/

s/achieves that same result with/requires code like the following for the same result (less efficiently):/

s/seems the only/is the only/

__________
Better Conversion-Failure Check With convert<>::result

Use a code/typewriter face for types, variable names, literals, etc. in the prose.  For example, "'i2'" should be something like "<CODE>i2</CODE>" instead.

The first sentence should not discuss the code below except by way of introduction.  Thus:

Consider the following code:

   int const i(convert<int>::from("not an int", -1);
   bool const conversion_failed(i == -1);

<CODE>i</CODE> will be <CODE>-1</CODE> after the attempted conversion, so <CODE>conversion_failed</CODE> will be true.

The second paragraph can then be improved by simplifying it and being more direct:

Checking the returned value is straightforward, but not always correct.  The problem is that <CODE>-1</CODE> is also the result of converting <CODE>"-1"</CODE> to <CODE>int</CODE>. Consequently, the fallback value must be selected carefully for each use case.  There are many cases in which there are unused values of the target type that can be used as the fallback, so this form of conversion is useful and convenient in those cases.

The third paragraph seems like an unfinished thought.  "More so" is just out of place.  Combining the third, fourth, and fifth paragraphs would be helpful:

While it can be appropriate to use fallback values, thereby ignoring conversion failures, there are use cases in which conversion failures must be detected.  In such cases, throwing an exception on conversion failure may be appropriate as illustrated in the following example:

s/ (not to mention the heaviness of the try/catch interface which does not seem exactly fitting on such a low level)/. Not only are exceptions expensive, but even writing a try block makes the code verbose and less clear than it could be/

s/More so, some classes might fail to meet that requirement for the Target type to be DefaultConstructible/What's worse, some classes don't meet the Target type's DefaultConstructible requirement/

s/More so, the following is no good either as it does not provide a reliable detection of a conversion failure/The following illustrates that there is no useful fallback value to indicate conversion failure/

s/For situations like that/To solve such problems,/

s/That same convert::result could be deployed to work around the throwing behavior of the lexical_cast-like interface/convert::result also provides a means to avoid conversion failure exceptions/

__________
Two Behavioral Policies

"The examples above demonstrate two interfaces behave differently with regard to handling conversion failures."

There are no examples "above," so this statement is misplaced.  Instead, change the first paragraph to the following:

There are two ways that Boost.Convert handles conversion failures:

s/when the fallback/when a fallback/

s/these behavior but only delay their application or execution/these behaviors; it merely delays them/

The last sentence can be made clearer:

That is, attempting to retrieve a value from a convert::result returned by a failed conversion will throw an exception or return the fallback value, depending on the conversion interface used.

__________
Requirements on the Argument and Result Types

The first sentence is awkward.  I suggest the following:

Presently, string-to-type and type-to-string conversions are based upon std::stream (like boost::lexical_cast).  That implies the following requirements for the source and destination types:

s/TypeIn is OutputStreamable of a string-related type - a char or wchar_t-based container compatible with the boost::range concept/TypeIn must be OutputStreamable; TypeIn must be a char- or wchar_t-based type type compatible with Boost.Range/

s/TypeOut is CopyConstructible./TypeOut is CopyConstructible;

__________
Integration of User-Defined Types

s/uniformly via boost::convert interface/uniformly via boost::convert/

__________
Modifying Throwing Behavior

The first two sentences are misplaced.  The page is about modifying throwing behavior, not noting that one might be forced to use the exception throwing interface.  However, the problem with this page is bigger.  The first way to "modify throwing behavior" is simply to use an interface that *prevents* Boost.Convert from throwing an exception.  (How client code chooses to react to a failure, including throwing the client's exception type, isn't relevant.)

The real point of this page, it appears, is to show that a type without a default constructor can still be used and trigger an exception on conversion failure.  Consequently, this is likely a better introductory paragraph:

When converting from a type without a default constructor, it is sometimes necessary to have conversion failures trigger an exception rather than use a fallback value.  To do that, use Boost.Convert's <CODE>throw_</CODE> directive:

With that change, remove the initial code block and the following "or alternatively" line.

s/forces to throw/causes Boost.Convert to throw/

__________
Formatted Conversions

s/by deploying the standard/by using the standard/

s|// This call fails|// This conversion fails, so the fallback is used|

s/is no better (or worse)/performs no better (or worse)/

s/formatting. In fact, under the hood it is std::stream-based/formatting, because it uses std::stream to do the formatting/

s/one should be looking at deploying comprehensive parsing and formatting libraries of their choice (say, Boost.Spirit might be a worthy contender)/one should consider using comprehensive parsing and formatting libraries like those in Boost.Spirit/

s/Still, one does not need a big cannon for shooting ducks (not that I approve such an activity) as one does not need a comprehensive formatter when all that is needed is/Still, the simplicity of Boost.Convert is often good enough/

__________
Locale-Based Conversions

s/Locales are deployed in a similar fashion/Using locales with Boost.Convert is easy as shown by this example/

s/For the current locale being "en_AU" the/Assuming the current locale is "en_AU," the/

__________
String-To-String Conversions

The first paragraph is an apology for the inclusion of a feature that shouldn't be documented as part of the reviewed library or else should be documented as an official part of the library (which reviewers could, of course, reject).  Therefore, the first paragraph should be more like the following:

Boost.Convert provides support for string-to-string conversions as illustrated with the following code which converts among different encodings:

s/More so, a more practical (and not as remote) deployment of the string-to-string conversion might be demonstrated by the following snippet (taken from the library unit test)/The following code, taken from the unit_test library, illustrates how Boost.Convert's string-to-string conversions can be used for encrypting and decrypting a string/

s/Where my_cypher is a custom manipulator with the following signature/To enable such conversions, a custom manipulator, my_cypher, is needed:/

s/With the above-mentioned manipulator applied boost::convert keeps the type (std::string) of the source but changes (encrypts or decrypts) its representation/my_cypher encrypts or decrypts a string (without changing its encoding)/

__________
Using Boost.Convert with Standard Algorithms

The introductory paragraph can be improved to something like this:

Both conversion interfaces can be used with standard algorithms and work as expected:

The example code warrants more discussion since it relies on non-obvious function objects.  Clearly, convert<>::from<> is a function object type, but I don't recall that having been discussed previously.  Even if it was, it warrants discussion in this page because of its relevance to use with standard algorithms.  Consequently, the introductory paragraph might be more like this:

Both conversion interfaces can be used with standard algorithms.  This is possible because there are function objects underlying the behavior of Boost.Convert.  Consider the following code:

Then, you can call out the three function objects created in the example code and discuss their type and calling convention, etc.

__________
Accessing Converters Directly

The introductory paragraph is almost nonsense.  I think that your intent on this page is to discuss the function objects, called _converters_ apparently, the discussion of which should precede using standard algorithms and would have precluded my comments on that section save for the need to reference this section to understand the function object behavior.

Here's what I suggest for this page (which would replace all text on the page):

What powers Boost.Convert are the function objects created by calling convert<Target>::from<Source>().  Those function objects, known as _converters_, can be saved for reuse, passed to a function or algorithm, or used as an unnamed temporary in the most straightforward uses of Boost.Convert.

A _converter_ has the type convert<Target>::converter<Source>.  The following code illustrates creating and reusing them in a single context.  Refer to _Using Boost.Convert with Standard Algorithms_ for examples of using them with algorithms.

__________
More About Converters, Customization, Optimization and Performance

The text on this page is awkard.  Here's a possible rewrite to consider:

_Accessing Converters Directly_ and _Using Boost.Convert with Standard Algorithms_ discuss _converters_ and their uses in various contexts.  Given that _converters_ are the heart of Boost.Convert, it should be no surprise that most of the configurability and capabilities of the library are found therein.  For example, manipulator support and support for directives like throw_ and fallback_ are provided by _converters_.

In the current Boost.Convert implementation, there is a generic string-to-type converter with an optimized specialization for string-to-bool conversions.  That specialization does not support manipulators and is not std::stream-based, demonstrating that _converters_ need support only what is deemed necessary for a particular case.

New _converters_ can be created for any Source/Target conversion pair focused on functionality or performance, as need be, and Boost.Convert will use the specializations without affecting the high level interfaces.

_____
Rob Stewart                           robert.stewart_at_[hidden]
Software Engineer                     using std::disclaimer;
Dev Tools & Components
Susquehanna International Group, LLP  http://www.sig.com

IMPORTANT: The information contained in this email and/or its attachments is confidential. If you are not the intended recipient, please notify the sender immediately by reply and immediately delete this message and all its attachments. Any review, use, reproduction, disclosure or dissemination of this message or any attachment by an unintended recipient is strictly prohibited. Neither this message nor any attachment is intended as or should be construed as an offer, solicitation or recommendation to buy or sell any security or other financial instrument. Neither the sender, his or her employer nor any of their respective affiliates makes any warranties as to the completeness or accuracy of any of the information contained herein or that this message or any of its attachments is free of viruses.