Vladimir Batov wrote on Saturday, May 24, 2014 9:40 PM

...

Habraken wrote The most trivial use of boost::convert, and boost::lexical_cast and boost::coerce simply don't compare in my opinion.

boost::cstringstream_converter cnv; int i = boost::convert <int>::from("23", cnv).value();

versus

int i = boost::lexical_cast<int>("23"); int j = boost::coerce::as<int>("23");

Here we go again... :-) lexical_cast will be haunting me to my grave. :-)

For starters, please stop using that silly lexical_cast deployment example as our benchmark. Such deployment does not exist. >lexical_cast cannot possibly be deployed like that in any real application. The correct and responsible deployment is

int i = -1; int j = -1;

try { i = boost::lexical_cast<int>(str); } catch (...) {} try { j = boost::lexical_cast<int>(str); } catch (...) {}

which is not exactly the two-liner that you sited! Compared to that the "convert" usage is

boost::cstringstream_converter cnv; int i = boost::convert<int>(str, cnv).value_or(-1); int j = boost::convert<int>(str, cnv).value_or(-1);

does look simple and intuitive enough IMO, configurable and potentially more efficient. (In the post_review branch the >convert::from API has been retired).

I guess I'm silly, but such deployment does exist in my code base, to good effect. Convert "something" to "something else". If it doesn't work, throw an exception. That's a typical C++ idiom. Any implementation that requires some cryptic object-creation-before-I-call-a-function doesn't make sense to me. Your example adds default-value functionality, which is usually (in my use cases) inappropriate.

...

When the convert library makes it into boost I'd want to mentally deprecate lexical_cast as a library I should use, and always use convert instead. This means convert should have a default converter, which may well be backed by lexical_cast with an interface such as:

int i = boost::convert<int>::from("23").value();

This still doesn't completely satisfy me as I'd like to write

int i = boost::convert<int>::from("23");

Again, I can't possibly stress more that the (deceivingly simple) usage examples you insist on do not exist in real >applications -- they are functionally-incomplete (as they do not handle all possible use-cases). Still, with the post_review branch one can responsibly(!) write

int i = boost::convert<int>(str, cnv).value_or(-1);

I can't possibly stress more that those cases do exist. I know that in my application, I almost never know some default value_or, so if the conversion doesn't work, I *want* an exception. Forcing the programmer to decide appropriate default values for every conversion isn't silly- I don't know, I just want a valid conversion or an exception. At any rate, I largely agree with Jeroen's points, and I find the Convert interface excessively complex. Specifically, if I have to create a conversion object that I then immediately pass as an argument to a conversion function, that seems excessively complex to me. Erik ---------------------------------------------------------------------- This message, and any attachments, is for the intended recipient(s) only, may contain information that is privileged, confidential and/or proprietary and subject to important terms and conditions available at http://www.bankofamerica.com/emaildisclaimer. If you are not the intended recipient, please delete this message.

Hi, Having slept on this last night, and with a fresh mug of coffee in hand let me see if I can clarify some things below. On 25 May 2014 03:40, Vladimir Batov wrote:

Jeroen, Thank you for your review it is very much appreciated. And, indeed, by your own admission you've "mellowed a bit since" a few years ago. Still, I see some bits left of "so strongly disagree"... should I have waited for a little bit longer? :-)

My first reaction was that it was hard to say, and only time would tell. On second thought however I think you should not only have waited a little longer, but gotten a few real world users. It's very hard to argue against three to five real world users who are using your code in production stating they want it in boost. At that point most of the obvious implementation bugs should have been ironed out and they're convinced of your interface to the point of actually using it. That's testament! That said I'm well aware this isn't easy, I was delighted to have boost::coerce reviewed in [4] without my input. At times a mail would also pop up complaining my library stopped working in the latest version of, surprise, MSVC. I have no idea who's using it though unfortunately. Jeroen Habraken wrote

...

Hi,

As promised my review of the boost::convert library. Note that the questions were answered in a seemingly random order, thus I hope it all comes together. Secondly, being the author of the boost::coerce library [2][3] I may be a little biased at times, please forgive me.

Now that you mention "coerce" I do remember you mentioning it back then during review #1. I completely forgot about it and I still can't see it in Boost. Now when I look for "coerce" Google only gives 2012 links. :-(

I've been meaning to put the finishing touches on it and get it reviewed but then life happened, and as I'm sure you're perfectly aware by now these reviews are no walk in the park.

-- What is your evaluation of the design?

...

There are two parts to this, the design of the backend and the design of the interface that the user predominantly sees. I have little to say on the first, the notion of multiple backend converters is a big step forwards and their interface is sane.

I'm far less convinced when it comes to the user-facing interface as I believe the simple tasks should be simple and intuitive, whereas the harder tasks may be harder.

The most trivial use of boost::convert, and boost::lexical_cast and boost::coerce simply don't compare in my opinion.

boost::cstringstream_converter cnv; int i = boost::convert <int> ::from("23", cnv).value();

versus

int i = boost::lexical_cast <int> ("23"); int j = boost::coerce::as <int> ("23");

Here we go again... :-) lexical_cast will be haunting me to my grave. :-)

For starters, please stop using that silly lexical_cast deployment example as our benchmark. Such deployment does not exist. lexical_cast cannot possibly be deployed like that in any real application. The correct and responsible deployment is

int i = -1; int j = -1;

try { i = boost::lexical_cast<int>(str); } catch (...) {} try { j = boost::lexical_cast<int>(str); } catch (...) {}

which is not exactly the two-liner that you sited! Compared to that the "convert" usage is

boost::cstringstream_converter cnv; int i = boost::convert<int>(str, cnv).value_or(-1); int j = boost::convert<int>(str, cnv).value_or(-1);

does look simple and intuitive enough IMO, configurable and potentially more efficient. (In the post_review branch the convert::from API has been retired).

This is a fair point but as other have argued boost::lexical_cast is used without the try-catch in a lot of places. Simply expecting it not to throw and dropping everything on the ground when it does isn't the nicest way to go about things but it does get things done. I guess the question is whether we should make it harder to use the library in such a way, and I think we shouldn't. If a users wants to potentially blow off a foot that's their call.

When the convert library makes it into boost I'd want to mentally

...

deprecate lexical_cast as a library I should use, and always use convert instead. This means convert should have a default converter, which may well be backed by lexical_cast with an interface such as:

int i = boost::convert <int> ::from("23").value();

This still doesn't completely satisfy me as I'd like to write

int i = boost::convert <int> ::from("23");

Again, I can't possibly stress more that the (deceivingly simple) usage examples you insist on do not exist in real applications -- they are functionally-incomplete (as they do not handle all possible use-cases). Still, with the post_review branch one can responsibly(!) write

int i = boost::convert<int>(str, cnv).value_or(-1);

I think I can live with that idea of using lexical_cast-based converter by default... However, I personally favor explicitness/readability and, therefore,

int i = boost::convert<int>(str, boost::lexical_cast_converter()).value_or(-1);

Here I disagree but this is a matter of taste. Let me propose the following, a customisation point where users can choose their default converter within their application, defaulting to the lexical_cast_converter. This prevents people from having to instantiate a converter everywhere, either explicitly or inline, and it gives them the option to switch the converter for their whole application in a single place.

but it'd be an improvement in my opinion. Now I'm aware lexical_cast

...

doesn't have a non-throwing interface, and it probably never will, but I've solved the problem as follows in boost::coerce:

int i = boost::coerce::as_default <int> ("XXX"); int j = boost::coerce::as_default <int> ("XXX", tag::hex()); int k = boost::coerce::as_default <int> ("XXX", 23); int l = boost::coerce::as_default <int> ("XXX", tag::hex(), 23);

The former two returning the default-constructed int, 0,

1) For the first 2 deployments: I personally cannot justify the former two returning int(). The questionable perk of not typing a few more characters comes at the expense of introducing considerable ambiguity and potential misinterpretation.

2) For all 4 deployments: Again, no matter how temping it is to have a "simple" interface like that (similar to lexical_cast), it simply does not cut the mustard (so to speak) in real life... simply because it is not complete. Namely, it is non-deterministic:

int k = boost::coerce::as_default<int>("-1", -1);

...

the latter two returning 23. That said, I'll be the first to admit the coerce interface isn't perfect, the fact that I need SFINAE to distinguish between as_default calls with two parameters, the second either being a tag (somewhat similar to your converters but not quite) or the default value goes to show just that. However I'd prefer

int i = boost::convert <int> ::from("23", cnv); int i = boost::convert <int> ::from_default("23", cnv, 0);

to the current interface using value_or(0). This is subjective though, and quite possibly a matter of taste.

The "convert" signature (I keep referring to the post_review branch) is actually dead simple:

std::tr2::optional<TypeOut> convert(TypeIn const&, Converter const&);

The rest, namely, value(), value_or(), throwing behavior come from std::tr2::optional. I personally find its interface very intuitive. I do understand that you insist on a different interface for "convert"... as in lexical_cast and "coerce"... however, as I stated in the docs, in real life that API makes quite a few legitimate process/program flows difficult and awkward to implement.

...

In summary, before this turns into too long a rant, I think the current user-facing interface can be improved substantially, but I'm well aware this is subjective.

Yes, I believe it is crucially important we come up with process-flow-complete or functionally-complete(!) API. So far,

std::tr2::optional<TypeOut> convert(TypeIn const&, Converter const&);

seems the best. I tried to explain why alternative examples you provided do not seem to always work.

Let me respond to this below.

...

-- What is your evaluation of the implementation? ...

Snipped for brevity. I agree with all mentioned points and will be addressing them if we decide to go ahead with "convert".

In my defence it all was put together in haste (especially converters). What is *actually* important to me at this stage is nailing the "convert" interface which so far is

std::tr2::optional<TypeOut> convert(TypeIn const&, Converter const&);

If we decide to go ahead with "convert", I'll be addressing all comments so far to the best of my abilities. Putting converters into boost::converter namespace sounds like a good idea... and I am hoping that people with more specific knowledge step in and provide excellent converters catering for different purposes... like coerce-based? :-)

As said most of it was minor at best, and I wholeheartedly agree nailing the interface is the most important at this point. It worries me a little that the converters were put together in haste, these things shouldn't be rushed.

And finally:

...

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

In my previous review this was a straight no, stating that the functionality should be added to boost::lexical_cast. With the current design supporting multiple backends this no longer holds true as I've stated in the evaluation of the potential usefulness of the library.

Again, the library is quite useful, but, I so strongly disagree with the current user-facing interface that I can't straight-out accept it. Once the library is accepted this interface is set in stone and like marriage it's "speak now or forever hold your peace".

As such I'm voting YES, CONDITIONALLY on the interface being simplified for the seemingly simple cases as most of my other comments were minor or quite trivial to fix.

Well, I personally do not believe "speak now or forever hold your peace" holds true and stops any potential improvements if/when necessary. Given your familiarity with Spirit I'd expect you to be aware of it going through major incompatible revisions over time. And Spirit is not an exception here.

That's true, however such a major revision should not be required within the first years of inclusion.

Here, again, IMO the "convert" interface is the center-piece of "convert" design. Converters' signature, et al. are secondary and can change without affecting users (much). The "convert" interface is crucial. So, if you "so strongly disagree" with it, you cannot possibly vote "yes"... even conditionally... especially when your "condition" is to abandon the existing design. :-) Having said that I feel it needs to be pointed out that saying "I do not like it" without providing a suitable alternative is not sufficient. Unfortunately, so far I have not seen any serious alternatives to the existing proposed "convert" API. Please do not get me wrong. Give us an interface covering all use-cases (hopefully mentioned in the docs). If community decides it's better, I'll incorporate it.

Let me first say that I probably worded the "so strongly disagree" too harshly, and note that I didn't vote against the library, just the current interface. This is getting to the meat of the discussion though and looking back there are two distinct points I'm trying to make. First of all I think there should be a default converter, not requiring people to explicitly construct it everywhere. Above I proposed a customisation point where people can select the default converter for their project, defaulting to the boost::lexical_cast based converter. This feels like a fair middle-ground to me offering a more powerful interface to the user, and we can have the bike shed discussion on which converter it should default to. The lexical_cast one makes sense to me. Secondly I'm not a big fan of the .value() and .value_or(-1) interface but I can live with this. Especially since the std::optional also has the operator* and operator-> allowing int i = *boost::convert<int>::from("23"); which will make sense when people become more accustomed to std::optional. Note that for this to be efficient I'd like to see the optional to be movable, which isn't yet the case with boost::optional if I'm not mistaken.

...

interested. Let's have a look at whether it may be possible to merge the functionally of boost::convert and boost::coerce, as they seem to have grown towards each other with the possibility to support multiple backends. Two years ago I'd never have proposed this, but I've mellowed a bit since then.

You seem to have focused on backends primarily backed by std::stringstream and friends whereas I have focused on a backend backed by boost::spirit. These may end up complementing leading to a single library capable of supporting both, being more generic and powerful to the user when done right.

Regardless of whether we end up going this way I'd love to have a discussion on the interface, either on the mailing list or offline as

With that out of the way I'd like to propose the following if you're this

...

is currently a major stumbling block for me.

Work together? Sure. Again, I actually see this proposal as an attempt to gather all input that I might incorporate for everyone's benefit. So, in this review I am not a "visionary" but the requirements gatherer and implementer. :-) For me it's not about ego but about having something community-accepted and standardized (I am a sucker for standardization and unification and homogenization :-))

That said, you might like to soften a bit your "so strongly disagree" stance. :-) Everyone brings their designs to the table. We pick the most functionally-complete. Nothing personal.

As said above that was worded unnecessarily harsh, and wasn't mean that way.

What I see important at this stage is to settle on the "convert" user API. Currently,

std::tr2::optional<TypeOut> convert(TypeIn const&, Converter const&);

Then, I'd focus on std::sstream-based converter and you might bring coerce-based one for everyone's benefit. What do you think?

Exactly, once we nail the interface this is where we should be heading. Since you mentioned rushing the converters I suspect they may need a little love, and I can definitely integrate boost::coerce into boost::convert. I should expand on this as I don't mean simply adding a converter which calls coerce but fully porting over its functionality to convert. At some point I argued that coerce its functionality may be added to boost::spirit, but this would be a better home.

The discussion of the converter signature would be close second on the list. I am resisting but inevitably drifting towards the signature Alex proposed:

void operator()(TypeIn const&, std::tr2::optional<TypeOut>&);

as it seems potentially more efficient and less restrictive than current

bool operator()(TypeIn const&, TypeOut&);

and my attempts to deploy

std::tr2::optional<TypeOut> operator()(TypeIn const&);

signature fail to cover all deployment cases so far.

Your contributions are most welcome.

I'm slowly warming up to the optional-based interface as it eliminates the throwing / non-throwing interface discussion, which is a good thing. Once again two comments, first of which is what's going to happen until the std::optional (whether in TR2 or elsewhere) is widely accepted. The optional provided by boost has a slightly different interface and isn't movable. The second is performance, with boost::coerce a lot of time was invested to prevent the interface from slowing down the conversion itself. I simply don't know how this interface will behave in that regard, when I have more time I'll try some benchmarking. Jeroen [4] http://www.kumobius.com/2013/08/c-string-to-int/

On 25 May 2014 14:03, Vladimir Batov wrote:

Jeroen, Thank you for your input. My apologies for snipping (a lot) -- I want to focus on the immediate. If we clear this one up, we'll get to the rest next. If we do not clear this one up, we won't get to the rest. :-)

You're more than welcome, you may have snipped a little too much but I'll get to that below.

...

...

as other have argued boost::lexical_cast is used without the try-catch in a lot of places. Simply expecting it not to

Jeroen Habraken wrote throw

...

and dropping everything on the ground when it does isn't the nicest way to go about things but it does get things done.

Jeroen, seriously, we are serious people writing serious s/w. Should we be discussing children playing with matches here? :-)

Unfortunately I believe we should, whilst we might be writing serious software there are enough who aren't -- you'd be surprised how often people are using boost::lexical_cast without the try - catch at the place I currently work. Again, unfortunately.

...

I guess the question is whether we should make it harder to use the library in such a way, and I think we shouldn't.

It is the wrong angle IMO. First, we should make it work properly. Then we make it as simple to use as possible within the constraints imposed by the first point.

Agreed.

...

Note that for this to be efficient I'd like to see the optional to be movable, which isn't yet the case with boost::optional if I'm not mistaken.

You might have noticed that I mention std::tr2::optional... which has what you are after. Andrzej is working on incorporating/backporting those features to boost::optional. "Life happened" to him also :-) but I am hoping, if we are extra nice, he might just might to get the interface we are after into 1.56. Andrzej, ple-e-e-ease, pretty pretty please!?

Wonderful, I'd love to see that make it into 1.56.0.

...

I'm slowly warming up to the optional-based interface as it eliminates the throwing / non-throwing interface discussion, which is a good thing. Once again two comments, first of which is what's going to happen until the std::optional (whether in TR2 or elsewhere) is widely accepted. The optional provided by boost has a slightly different interface and isn't movable.

It is

std::tr2::optional<TypeOut> convert(TypeIn const&, Converter const&);

you are warming up to, right? If it is so, I am really glad to hear that. I am not married to it but all things considered it seems the most generic and flexible, functionally-complete, not too verbose... and sufficiently close to lexical_cast :-) for people not to feel alienated.

Given the immense variability of conversion-related deployments I see this interface as the only one that everyone can deploy -- exceptions? no problem; delayed exceptions? easy; no exceptions but failure-condition returned instead? piece of cake; can't be bothered processing the failure-condition and wanna proceed with the default? coming right up; want to process failure but still go with the default? anything to keep the user happy!

So far, no one (to my knowledge) has come up with another interface which simpler without sacrificing those mentioned important qualities (given we are talking about a library-grade interface).

Close, I'd like an overload without the Converter const & as well, defaulting to a converter chosen via a customisation point. This is what I believe you've unintentionally snipped, and I'm curious as to your opinion on it as I believe it to be a crucial part of the interface.

...

The second is performance, with boost::coerce a lot of time was invested to prevent the interface from slowing down the conversion itself. I simply don't know how this interface will behave in that regard, when I have more time I'll try some benchmarking.

Ah, that should not be a problem. I don't think. I actually expect the convert() interface to be wiped out altogether when optimized. That is, all power to you, the converter writer. So, what do you say? Are we in business?.. So to speak? :-)

We're getting there. Jeroen

First of all, let me tip my hat to Jeroen for his very thorough and thoughtful review. I fully agree with all what he said. Let me also get out of the way that I have not looked at the library itself. 'm just following the discussions.

Jeroen, Thank you for your input. My apologies for snipping (a lot) -- I want to focus on the immediate. If we clear this one up, we'll get to the rest next. If we do not clear this one up, we won't get to the rest. :-)

...

...

as other have argued boost::lexical_cast is used without the try-catch in a lot of places. Simply expecting it not to

Jeroen Habraken wrote throw

...

and dropping everything on the ground when it does isn't the nicest way to go about things but it does get things done.

Jeroen, seriously, we are serious people writing serious s/w. Should we be discussing children playing with matches here? :-)

A large part of the software I write (which is no child's play, at least in my eyes) needs simple solutions to complex problems. The simpler a solution the better. I as a library writer often leave the handling of exceptions which are unrelated to the task at hand to the user of my library. Thus having a conversion throw an exception deep in the guts of my library is a perfect solution. This is particularly true if my library has no way of telling what would be a proper default value. I might be biased as I feel to have my share in the interface design of Jeroen's Coerce library. From that experience (and all the other library work I'm doing) I wouldn't like to have library interfaces which are more verbose than absolutely needed. That said, I very much like the interface design of Coerce, as it is as straight as it can get for the common case and allows to customize its behavior with little effort, without me having to remember shrouded syntax. Also, as Jeroen stated, library interfaces should not get in the way of performance. But more about this further down.

...

I guess the question is whether we should make it harder to use the library in such a way, and I think we shouldn't.

It is the wrong angle IMO. First, we should make it work properly. Then we make it as simple to use as possible within the constraints imposed by the first point.

...

Note that for this to be efficient I'd like to see the optional to be movable, which isn't yet the case with boost::optional if I'm not mistaken.

You might have noticed that I mention std::tr2::optional... which has what you are after. Andrzej is working on incorporating/backporting those features to boost::optional. "Life happened" to him also :-) but I am hoping, if we are extra nice, he might just might to get the interface we are after into 1.56. Andrzej, ple-e-e-ease, pretty pretty please!?

Honestly, when I first realized your convert() returns an optional I felt something like revulsion. This is for two reasons: a) While an optional exposes an operator*() and an operator->(), the resulting conversion syntax turns into something more complex than it has to be. b) Using optional means you need an additional allocation (at least sometimes if you have an improved version using small object optimization internally). I tend to say that this alone can turn into a knockout criteria for a supposedly high performance, low level conversion library. However I have not done any measurements - so all this is pure conjecture. There have been done plenty of conversion library comparisons by different people. It might be a good idea to show performance results of your solution comparing to the existing libraries.

...

I'm slowly warming up to the optional-based interface as it eliminates the throwing / non-throwing interface discussion, which is a good thing. Once again two comments, first of which is what's going to happen until the std::optional (whether in TR2 or elsewhere) is widely accepted. The optional provided by boost has a slightly different interface and isn't movable.

It is

std::tr2::optional<TypeOut> convert(TypeIn const&, Converter const&);

you are warming up to, right? If it is so, I am really glad to hear that. I am not married to it but all things considered it seems the most generic and flexible, functionally-complete, not too verbose... and sufficiently close to lexical_cast :-) for people not to feel alienated.

Given the immense variability of conversion-related deployments I see this interface as the only one that everyone can deploy -- exceptions? no problem; delayed exceptions? easy; no exceptions but failure-condition returned instead? piece of cake; can't be bothered processing the failure-condition and wanna proceed with the default? coming right up; want to process failure but still go with the default? anything to keep the user happy!

So far, no one (to my knowledge) has come up with another interface which simpler without sacrificing those mentioned important qualities (given we are talking about a library-grade interface).

See Jeroen's Coerce library for an example. It's as simple as lexical_cast or as complex as a full blown Spirit parser and everything in between.

...

The second is performance, with boost::coerce a lot of time was invested to prevent the interface from slowing down the conversion itself. I simply don't know how this interface will behave in that regard, when I have more time I'll try some benchmarking.

Ah, that should not be a problem. I don't think. I actually expect the convert() interface to be wiped out altogether when optimized.

I used to think that as well. Sadly, more often than not compilers are not able to optimize all of it. But there is hope that they will get better over time. At least I would like to see numbers confirming your statement. Regards Hartmut --------------- http://boost-spirit.com http://stellar.cct.lsu.edu

On 05/26/2014 11:32 AM, Vladimir Batov wrote:

I did. I do not believe it covers all deployment scenarios. I can be wrong though. That said, I have to qualms stepping back and letting Jeroen to bring "coerce" back to life. I'll take someone else's work any time (with commercial-development-specifics caveats). Less work and hassle for me. Honestly, it'll be a relief. I am already exhausted just going through this 2 weeks review.

Meant to say "I have NO qualms stepping back". In fact, I am beginning to think that I might have to retreat defeated (no drama). It is because, if my memory serves me, the majority of reviews had "conditional yes" votes which seems good on the surface of it. The complication (as I see it) though is that those conditions were along the lines "I vote yes if you change the interface to what I like"... which renders "no" for the interface under consideration. I am happy to keep going (if decided so) but, truth be told, only a couple of reviews were in support of proposed interface and the rest were essentially "no"s due to "un-meet-able" :-) conditions... unless we can sum-up all the interface-related input into some concrete function signature... which I am happy to implement.

On 05/26/2014 12:44 PM, Hartmut Kaiser wrote:

Volodya,

...

Thank you for your input. It's much appreciated. From the critical tone of your post I'll take it you are voting "no". If you did not state it by accident, then I am happy to do that for you. If you did not state it intentionally, then I think you should as you should not be shying away from your community responsibilities. :-) Please find my replies below. I intentionally have not cast a vote as I don't have put enough effort into a proper review. My main goal was to support Jeroen in his review.

In any case, I wanted to mention a possible alternative to the design using optional<>. Boost.Filesystem and Boost.Asio give a nice precedent on how to construct a library interface supporting both, throwing and non-throwing functions without introducing penalties for either case (based on Boost.System). Using something similar for Convert seems to be a good idea.

I seem to be managing slowly pulling Jeroen onto my "dark" side :-) but his "conditional yes" vote so far is on the condition of "the interface being simplified for the seemingly simple cases"... which echoes very much with your reservations/revulsions ;-) regarding "optional" deployment. So, so far, IMO his vote can only responsibly be counted as "no"... given that everything else revolves around the proposed interface... that you and Jeroen... and quiet a few others are not exactly happy with... And that's perfectly fine by me. No drama. Thank you for the Boost.Filesystem and Boost.Asio pointers. Unfortunately, I am not familiar with the libs. If they indeed handle deployment cases similar to those I've been moaning about, then it might be wise to postpone going any further with "convert" and study those libs for already-existing/established solutions... without "optional"-generated controversy... Maybe someone else could shed more light on that?...

...

...

Thank you for your input. It's much appreciated. From the critical tone of your post I'll take it you are voting "no". If you did not state it by accident, then I am happy to do that for you. If you did not state it intentionally, then I think you should as you should not be shying away from your community responsibilities. :-) Please find my replies below. I intentionally have not cast a vote as I don't have put enough effort into a proper review. My main goal was to support Jeroen in his review.

In any case, I wanted to mention a possible alternative to the design using optional<>. Boost.Filesystem and Boost.Asio give a nice precedent on how to construct a library interface supporting both, throwing and non-throwing functions without introducing penalties for either case (based on Boost.System). Using something similar for Convert seems to be a good idea.

I seem to be managing slowly pulling Jeroen onto my "dark" side :-) but his "conditional yes" vote so far is on the condition of "the interface being simplified for the seemingly simple cases"... which echoes very much with your reservations/revulsions ;-) regarding "optional" deployment. So, so far, IMO his vote can only responsibly be counted as "no"... given that everything else revolves around the proposed interface... that you and Jeroen... and quiet a few others are not exactly happy with... And that's perfectly fine by me. No drama.

Thank you for the Boost.Filesystem and Boost.Asio pointers. Unfortunately, I am not familiar with the libs. If they indeed handle deployment cases similar to those I've been moaning about, then it might be wise to postpone going any further with "convert" and study those libs for already-existing/established solutions... without "optional"-generated controversy... Maybe someone else could shed more light on that?...

You usually add a parameter to your function like this: // global special error_code object (lives in Boost.System) error_code throws; void foo(error_code& ec = throws); then, inside foo() on error you do: if (error_happened) { if (&ec == &throws) // throw exception else { ec = make_my_error_code(); return; } } Which means, that if you use it as: foo(); it will throw on error. However, it will not throw if you write: error_code ec; foo(ec); if (ec) { // handle error } HTH Regards Hartmut --------------- http://boost-spirit.com http://stellar.cct.lsu.edu

2014-05-25 16:37 GMT+02:00 Hartmut Kaiser :

...

...

Note that for this to be efficient I'd like to see the optional to be movable, which isn't yet the case with boost::optional if I'm not mistaken.

You might have noticed that I mention std::tr2::optional... which has what you are after. Andrzej is working on incorporating/backporting those features to boost::optional. "Life happened" to him also :-) but I am hoping, if we are extra nice, he might just might to get the interface we are after into 1.56. Andrzej, ple-e-e-ease, pretty pretty please!?

Honestly, when I first realized your convert() returns an optional I felt something like revulsion. This is for two reasons:

a) While an optional exposes an operator*() and an operator->(), the resulting conversion syntax turns into something more complex than it has to be. b) Using optional means you need an additional allocation (at least sometimes if you have an improved version using small object optimization internally). I tend to say that this alone can turn into a knockout criteria for a supposedly high performance, low level conversion library. However I have not done any measurements - so all this is pure conjecture.

I just wanted to clarify one detail about the implementation of boost::optional. By design, it does not allocate heap memory for any type T, however big. It uses an internal aligned_storage-like facility to store the object. This aside, your point about performance may still be valid for other reasons (like returning by value an object bigger than necessary). I do not have a measurable evidence either. Regards, &rzej

Joel de Guzman wrote

On 5/25/14, 6:34 PM, Jeroen Habraken wrote:

...

... [4] http://www.kumobius.com/2013/08/c-string-to-int/

This makes me wonder. How fast is Convert BTW? How does it compare to the examples in that benchmark? Boost is performance hungry! It will be a real cause of concern if it is slower than the benchmarks.

"convert" has no benchmarks of it own... because it does nothing. :-) It's a "manager" class rather than a "worker" class. It's merely an interface to coordinate/unify and uniformly deploy (or quickly swap on when-needed basis) varying conversion facilities. So, with the "coerce"-based converter "convert" performs as fast as "coerce". I actually ran tests with lexical_cast and others: naked lex_cast vs. convert+lex_cast. No performance degradation. I mention it in the docs. -- View this message in context: http://boost.2283326.n4.nabble.com/review-Convert-library-tp4662821p4662880.... Sent from the Boost - Dev mailing list archive at Nabble.com.

On 26 May 2014 10:12, Vladimir Batov wrote:

Joel de Guzman wrote

...

On 5/25/14, 6:34 PM, Jeroen Habraken wrote:

...

... [4] http://www.kumobius.com/2013/08/c-string-to-int/

This makes me wonder. How fast is Convert BTW? How does it compare to the examples in that benchmark? Boost is performance hungry! It will be a real cause of concern if it is slower than the benchmarks.

"convert" has no benchmarks of it own... because it does nothing. :-) It's a "manager" class rather than a "worker" class. It's merely an interface to coordinate/unify and uniformly deploy (or quickly swap on when-needed basis) varying conversion facilities. So, with the "coerce"-based converter "convert" performs as fast as "coerce". I actually ran tests with lexical_cast and others: naked lex_cast vs. convert+lex_cast. No performance degradation. I mention it in the docs.

This will unfortunately not be the case if Hartmut is right, and Andrzej just confirmed he might be. The optional-based interface may actually introduce more in overhead than boost::coerce requires for the conversion. As said this needs to be backed by numbers, which is difficult without the std::tr2::optional in boost, but this may turn out to be a problem to me. Later versions of GCC are actually capable of optimizing boost::coerce::as<int>("23"); to the number 23 at compile time. I'd be a shame to have this obstructed by an optional. Jeroen

2014-05-26 11:08 GMT+02:00 Vladimir Batov :

Jeroen Habraken wrote

...

On 26 May 2014 10:12, Vladimir Batov <

...

vb.mail.247@

...

> wrote:

...

... I actually ran tests with lexical_cast and others: naked lex_cast vs. convert+lex_cast. No performance degradation. I mention it in the docs.

This will unfortunately not be the case if Hartmut is right, and Andrzej just confirmed he might be. The optional-based interface may actually introduce more in overhead than boost::coerce requires for the conversion.

As said this needs to be backed by numbers, which is difficult without the std::tr2::optional in boost, but this may turn out to be a problem to me. Later versions of GCC are actually capable of optimizing

boost::coerce::as <int> ("23");

to the number 23 at compile time. I'd be a shame to have this obstructed by an optional.

I do not see Andrzej confirming anything. He indeed asked a question... which a far from a confirmation of any sort. The way I see it

user-level optional<T> res = convert<T>(from, cnv);

the standard optimization/elision mechanism is to pass "res" to convert(). So, it becomes

convert(optional<T>& res, from, cnv);

I claim (without evidence in form of benchmarks) that returning optional<T> by value should not be slower than passing it as an input argument. Most compilers elide the moves/copies today, don't they? I would recommend making measurements before applying this optimization.

On 26 May 2014 11:08, Vladimir Batov wrote:

Jeroen Habraken wrote

...

On 26 May 2014 10:12, Vladimir Batov <

...

vb.mail.247@

...

> wrote:

...

... I actually ran tests with lexical_cast and others: naked lex_cast vs. convert+lex_cast. No performance degradation. I mention it in the docs.

This will unfortunately not be the case if Hartmut is right, and Andrzej just confirmed he might be. The optional-based interface may actually introduce more in overhead than boost::coerce requires for the conversion.

As said this needs to be backed by numbers, which is difficult without the std::tr2::optional in boost, but this may turn out to be a problem to me. Later versions of GCC are actually capable of optimizing

boost::coerce::as <int> ("23");

to the number 23 at compile time. I'd be a shame to have this obstructed by an optional.

I do not see Andrzej confirming anything. He indeed asked a question... which a far from a confirmation of any sort. The way I see it

user-level optional<T> res = convert<T>(from, cnv);

the standard optimization/elision mechanism is to pass "res" to convert(). So, it becomes

convert(optional<T>& res, from, cnv);

as Alex suggested converter has op()(TypeIn const&, optional<T>&) signature. So, the above calls

converter(from, res)

i.e. simply passes "res" straight to the converter. Inside the converter you fill that "res" with the result (potentially with move semantics). I do not see any extra-copying anywhere. More so, convert() is seemingly easily wiped out/inlined altogether. What am I missing?

Jeroen, do you think you might take the "convert" port_review branch and add your "coerce" test there? A couple of hours of community work? :-)

I may have time at the end of the week, but no promises unfortunately. Jeroen

Vladimir Batov wrote

Jeroen Habraken wrote

...

I may have time at the end of the week, but no promises unfortunately. OK, the old man has to do it all by himself. :-) How do I download the latest/greatest of "coerce" without copying individual files from the sandbox?

Or I'll just run raw Spirit.Qi example from http://www.kumobius.com/2013/08/c-string-to-int/ #include <string> #include #include bool String2Int(const std::string& str, int& result) { std::string::const_iterator i = str.begin(); if (!boost::spirit::qi::parse(i, str.end(), boost::spirit::int_, result)) return false; return i == str.end(); // ensure the whole string was parsed } -- View this message in context: http://boost.2283326.n4.nabble.com/review-Convert-library-tp4662821p4662894.... Sent from the Boost - Dev mailing list archive at Nabble.com.

Jeroen Habraken wrote

...

Vladimir Batov wrote

Or I'll just run raw Spirit.Qi example from http://www.kumobius.com/2013/08/c-string-to-int/

#include <string> #include <boost/spirit/include/qi_parse.hpp> #include <boost/spirit/include/qi_numeric.hpp>

bool String2Int(const std::string& str, int& result) { std::string::const_iterator i = str.begin(); if (!boost::spirit::qi::parse(i, str.end(), boost::spirit::int_, result)) return false; return i == str.end(); // ensure the whole string was parsed }

That's a nice solution, and should save quite a bit of headache.

Deployed the above code directly in a loop and via converter. Ubuntu 14.04. gcc 4.8.2. Compiled with -O3. Ran te test quite a few times: str-to-int spirit: raw/cnv=0.89/0.89 seconds. str-to-int spirit: raw/cnv=0.89/0.89 seconds. str-to-int spirit: raw/cnv=0.89/0.89 seconds. str-to-int spirit: raw/cnv=0.89/0.89 seconds. The code to review is in the post_review branch. Corrections welcome. -- View this message in context: http://boost.2283326.n4.nabble.com/review-Convert-library-tp4662821p4662901.... Sent from the Boost - Dev mailing list archive at Nabble.com.

On 5/26/14, 7:59 PM, Vladimir Batov wrote:

Vladimir Batov wrote

...

Jeroen Habraken wrote

...

...

Vladimir Batov wrote Or I'll just run raw Spirit.Qi example from http://www.kumobius.com/2013/08/c-string-to-int/

That's a nice solution, and should save quite a bit of headache. Deployed the above code directly in a loop and via converter. Ubuntu 14.04. gcc 4.8.2. Compiled with -O3. Ran te test quite a few times: str-to-int spirit: raw/cnv=0.89/0.89 seconds. str-to-int spirit: raw/cnv=0.89/0.89 seconds. str-to-int spirit: raw/cnv=0.89/0.89 seconds. str-to-int spirit: raw/cnv=0.89/0.89 seconds.

The code to review is in the post_review branch. Corrections welcome.

For comparison:

str-to-int spirit: raw/cnv=0.89/0.89 seconds. str-to-int: strtol/scanf/lcast/sstream=2.49/9.71/3.78/9.78 seconds.

impressive. Although getting spirit-related code to compile takes so-o-o-o many times longer. :-)

Yeah, the compiler will have to travel to the north pole and ask some elves to fully optimize the code ;-) In the end, it will be worth it. Promise! There's an even faster solution, but the result will always be 42. I'm not sure if you want that :-/ Regards, -- Joel de Guzman http://www.ciere.com http://boost-spirit.com http://www.cycfi.com/

On 2014-05-26 14:52, Vladimir Batov wrote:

Joel de Guzman wrote

...

...

impressive. Although getting spirit-related code to compile takes so-o-o-o many times longer. :-) Yeah, the compiler will have to travel to the north pole and ask some elves to fully optimize the code ;-) In the end, it will be worth it. Promise!

There's an even faster solution, but the result will always be 42. I'm not sure if you want that :-/

Regards, Joel de Guzman Can't stop laughing... I needed that. Thank you... "the result will always be 42". I love that.

I thought it took 7.5 million years to get that result? SCNR ;-)

On 5/26/14, 8:52 PM, Vladimir Batov wrote:

Joel de Guzman wrote

...

...

impressive. Although getting spirit-related code to compile takes so-o-o-o many times longer. :-)

Yeah, the compiler will have to travel to the north pole and ask some elves to fully optimize the code ;-) In the end, it will be worth it. Promise!

There's an even faster solution, but the result will always be 42. I'm not sure if you want that :-/

Regards, Joel de Guzman

Can't stop laughing... I needed that. Thank you... "the result will always be 42". I love that.

Seriously, both Qi and Karma have these numeric i/o conversions factored out in some low level numeric utility helpers that you can use directly without having to go to the expression template facade. Regards, -- Joel de Guzman http://www.ciere.com http://boost-spirit.com http://www.cycfi.com/

On 05/26/2014 04:26 PM, Joel de Guzman wrote:

On 5/25/14, 6:34 PM, Jeroen Habraken wrote:

...

That said I'm well aware this isn't easy, I was delighted to have boost::coerce reviewed in [4] without my input. At times a mail would also pop up complaining my library stopped working in the latest version of, surprise, MSVC. I have no idea who's using it though unfortunately.

[...]

[4] http://www.kumobius.com/2013/08/c-string-to-int/

This makes me wonder. How fast is Convert BTW? How does it compare to the examples in that benchmark? Boost is performance hungry! It will be a real cause of concern if it is slower than the benchmarks.

I wrote a half-hearted spirit-based converter and a performance test and got something I personally cannot explain. Can anyone help me to solve the puzzle? First, I compiled with -O3 using ~/dev/boost.convert/doc >> gcc --version gcc (Ubuntu/Linaro 4.6.3-1ubuntu5) 4.6.3 and get quite encouraging (even though hardly unexpected) results: str-to-int spirit: raw/cnv=0.28/0.28 seconds. str-to-int spirit: raw/cnv=0.28/0.28 seconds. where "raw" is a straight invocation of inline int str_to_int_spirit(std::string const& str) { std::string::const_iterator i = str.begin(); int result; BOOST_TEST(boost::spirit::qi::parse(i, str.end(), boost::spirit::int_, result)); BOOST_TEST(i == str.end()); // ensure the whole string was parsed return result; } and "cnv" is that same code wrapped-in as a converter. So far so good. No performance penalty for deploying conversion facilities via boost::convert(). Then though I tried that same code with gcc-4.8 on Ubuntu 14.04 32-bit architecture (oldish) machine... and got something I cannot get my head around of: str-to-int spirit: raw/cnv=1.44/1.41 seconds. str-to-int spirit: raw/cnv=1.44/1.41 seconds. That is, gcc-4.8 applies some magic so that that same code wrapped in a converter is 2% faster(!) than "raw" code. Is anyone curious and capable to solve the puzzle? I am admittedly the former but not the latter. :-( https://github.com/yet-another-user/boost.convert/blob/master/test/performan...

Matus Chochlik wrote

...

str-to-int spirit: raw/cnv=1.44/1.41 seconds. str-to-int spirit: raw/cnv=1.44/1.41 seconds.

That is, gcc-4.8 applies some magic so that that same code wrapped in a converter is 2% faster(!) than "raw" code. Is anyone curious and capable to solve the puzzle? I am admittedly the former but not the latter. :-(

https://github.com/yet-another-user/boost.convert/ blob/master/test/performance.cpp

IMHO this could have something to do with paging or the caching of the strings. You should test the two cases by two separate programs and try to run them both in the raw then cnv and cnv then raw orders. Or you could at least try to run

for (int k = 0; k < 3; ++k) printf("str-to-int spirit: raw1/raw2=%.2f/%.2f seconds.\n", performance_string_to_int_spirit(strings), performance_string_to_int_spirit(strings));

to try it out.

Matus, Geez, I would never figure that out by myself. Thanks. So, the first call would pull the strings into the fast cache so that the second call would not have to, right? Now I changed the test as int const num_tries = 20; double raw_time = 0; double cnv_time = 0; for (int k = 0; k < num_tries; ++k) raw_time += performance_string_to_int_spirit(strings); for (int k = 0; k < num_tries; ++k) cnv_time += performance::str_to_int(strings, boost::cnv::spirit()); printf("str-to-int spirit: raw/cnv=%.2f/%.2f seconds.\n", raw_time / num_tries, cnv_time / num_tries); Does it look fair to you? It surely gives more realistic results with bosst::convert() adding 0.7% overhead: 32:~/dev/boost.convert.>> ~/dev/bin/test-convert str-to-int spirit: raw/cnv=1.41/1.42 seconds. -- View this message in context: http://boost.2283326.n4.nabble.com/review-Convert-library-tp4662821p4663911.... Sent from the Boost - Dev mailing list archive at Nabble.com.

Matus Chochlik wrote

On Tue, Jun 10, 2014 at 8:58 AM, Vladimir Batov <

vb.mail.247@

> wrote:

...

...

IMHO this could have something to do with paging or the caching of the strings. You should test the two cases by two separate programs and try to run them both in the raw then cnv and cnv then raw orders. Or you could at least try to run

for (int k = 0; k < 3; ++k) printf("str-to-int spirit: raw1/raw2=%.2f/%.2f seconds.\n", performance_string_to_int_spirit(strings), performance_string_to_int_spirit(strings));

to try it out.

Matus,

Geez, I would never figure that out by myself. Thanks. So, the first call would pull the strings into the fast cache so that the second call would not have to, right? Now I changed the test as

int const num_tries = 20; double raw_time = 0; double cnv_time = 0;

Vladimir,

if this is indeed caused by caching and you want to run both tests in a single program, then you should do a "dry run" that will cause the strings to be cached before you do the actual timing. Also I'd increase the number of tries at least to 200.

...

for (int k = 0; k < num_tries; ++k) raw_time += performance_string_to_int_spirit(strings);

for (int k = 0; k < num_tries; ++k) cnv_time += performance::str_to_int(strings, boost::cnv::spirit());

printf("str-to-int spirit: raw/cnv=%.2f/%.2f seconds.\n", raw_time / num_tries, cnv_time / num_tries);

Does it look fair to you? It surely gives more realistic results with bosst::convert() adding 0.7% overhead:

32:~/dev/boost.convert.>> ~/dev/bin/test-convert str-to-int spirit: raw/cnv=1.41/1.42 seconds.

Well, it seems I spoke too soon. I was rushing out and tried the above in a hurry and, as a result, messed it up. Now I am doubtful it's cache-related as I simply run for (int k = 0; k < num_tries; ++k) { double raw_time1 = performance_string_to_int_spirit(strings); double raw_time2 = performance_string_to_int_spirit(strings); printf("str-to-int spirit: raw1/raw2=%.2f/%.2f seconds.\n", raw_time1, raw_time2); } and both raw_time1 and raw_time2 are the same (well, there is some variation but really minor and random). Then I run for (int k = 0; k < num_tries; ++k) { double cnv_time = performance::str_to_int(strings, boost::cnv::spirit()); double raw_time = performance_string_to_int_spirit(strings); printf("str-to-int spirit: raw/cnv=%.2f/%.2f seconds (%.2f%%).\n", raw_time, cnv_time, 100 * cnv_time / raw_time); } and regardless if I calculate cnv_time first or raw_time first I get the same results (i.e. the first call does not help the second call at all) as below: str-to-int spirit: raw/cnv=1.46/1.42 seconds (97.34%). str-to-int spirit: raw/cnv=1.47/1.42 seconds (96.15%). str-to-int spirit: raw/cnv=1.46/1.42 seconds (97.31%). str-to-int spirit: raw/cnv=1.45/1.42 seconds (97.45%). str-to-int spirit: raw/cnv=1.45/1.41 seconds (97.30%). I give up. My brain is too puny to understand that. -- View this message in context: http://boost.2283326.n4.nabble.com/review-Convert-library-tp4662821p4663936.... Sent from the Boost - Dev mailing list archive at Nabble.com.

Alex, I was planning on replying by Rob bit me to it pretty much expressing my view. My strong "attachment" to ref.-counted "Pimpl" is probably stems from the fact that I've always treated it as another/handier name for Handly/Body. Strangely, in all these years of deploying my-pimpl no one (including Sutter) "corrected" me on that... until now. :-( On 06/10/2014 11:43 PM, alex wrote:

...

Then I run

for (int k = 0; k < num_tries; ++k) { double cnv_time = performance::str_to_int(strings, boost::cnv::spirit()); double raw_time = performance_string_to_int_spirit(strings);

printf("str-to-int spirit: raw/cnv=%.2f/%.2f seconds (%.2f%%).\n", raw_time, cnv_time, 100 * cnv_time / raw_time); }

and regardless if I calculate cnv_time first or raw_time first I get the same results (i.e. the first call does not help the second call at all) as below:

str-to-int spirit: raw/cnv=1.46/1.42 seconds (97.34%). str-to-int spirit: raw/cnv=1.47/1.42 seconds (96.15%). str-to-int spirit: raw/cnv=1.46/1.42 seconds (97.31%). str-to-int spirit: raw/cnv=1.45/1.42 seconds (97.45%). str-to-int spirit: raw/cnv=1.45/1.41 seconds (97.30%).

I give up. My brain is too puny to understand that.

Where performance::str_to_int does

BOOST_ASSERT(parse() ), and BOOST_ASSERT(i == str.end())

performance_string_to_int_spirit does

if !parse() return false. return i == str.end()

Could it be that the assert function adds some overhead (passing the filename and linenumber), which explains the difference ?

Alex

_______________________________________________ Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost

On 06/10/2014 11:43 PM, alex wrote:

...

Then I run

for (int k = 0; k < num_tries; ++k) { double cnv_time = performance::str_to_int(strings, boost::cnv::spirit()); double raw_time = performance_string_to_int_spirit(strings);

printf("str-to-int spirit: raw/cnv=%.2f/%.2f seconds (%.2f%%).\n", raw_time, cnv_time, 100 * cnv_time / raw_time); }

and regardless if I calculate cnv_time first or raw_time first I get the same results (i.e. the first call does not help the second call at all) as below:

str-to-int spirit: raw/cnv=1.46/1.42 seconds (97.34%). str-to-int spirit: raw/cnv=1.47/1.42 seconds (96.15%). str-to-int spirit: raw/cnv=1.46/1.42 seconds (97.31%). str-to-int spirit: raw/cnv=1.45/1.42 seconds (97.45%). str-to-int spirit: raw/cnv=1.45/1.41 seconds (97.30%).

I give up. My brain is too puny to understand that. Where performance::str_to_int does

BOOST_ASSERT(parse() ), and BOOST_ASSERT(i == str.end())

performance_string_to_int_spirit does

if !parse() return false. return i == str.end()

Could it be that the assert function adds some overhead (passing the filename and linenumber), which explains the difference ?

I was surprised when you mentioned BOOST_ASSERT instead of BOOST_TEST. Now I see that I redefined BOOST_TEST to BOOST_ASSERT temporarily... and forgot to remove that. Thanks. And indeed BOOST_ASSERT seems to be heavier than BOOST_TEST due to expression-validity check done with __builtin_expect(expr, 1) when #define BOOST_TEST(expr) ((expr)? (void)0: ...) simply checks the "expr". Will try tonight with gcc-4.8 and report back. Thanks again.

On 6/11/14, 4:55 AM, Andrey Semashev wrote:

On Wednesday 11 June 2014 06:46:53 Vladimir Batov wrote:

...

And indeed BOOST_ASSERT seems to be heavier than BOOST_TEST due to expression-validity check done with

__builtin_expect(expr, 1)

It's not a validity check, it's a hint to the compiler to help branch prediction. Assertion failures are assumed to be improbable.

In any case, when testing performance you should be building in release mode, where all asserts are removed.

Benchmarks are a black art. See how we do our performance tests in Spirit: https://github.com/boostorg/spirit/blob/master/workbench/qi/int_parser.cpp You can use our benchmark facility where all the black art is contained: https://github.com/boostorg/spirit/blob/master/workbench/measure.hpp using this strategy: // Strategy: because the sum in an accumulator after each call // depends on the previous value of the sum, the CPU's pipeline // might be stalled while waiting for the previous addition to // complete. Therefore, we allocate an array of accumulators, // and update them in sequence, so that there's no dependency // between adjacent addition operations. // // Additionally, if there were only one accumulator, the // compiler or CPU might decide to update the value in a // register rather that writing it back to memory. we want each // operation to at least update the L1 cache. *** Note: This // concern is specific to the particular application at which // we're targeting the test. *** // This has to be at least as large as the number of // simultaneous accumulations that can be executing in the // compiler pipeline. A safe number here is larger than the // machine's maximum pipeline depth. If you want to test the L2 // or L3 cache, or main memory, you can increase the size of // this array. 1024 is an upper limit on the pipeline depth of // current vector machines. A naive test implementation will give you *funny* results, depending on the machine you are running on. HTH. Regards, -- Joel de Guzman http://www.ciere.com http://boost-spirit.com http://www.cycfi.com/

Joel, Joel de Guzman wrote

Benchmarks are a black art. See how we do our performance tests in Spirit:

https://github.com/boostorg/spirit/blob/master/workbench/qi/int_parser.cpp You can use our benchmark facility where all the black art is contained: https://github.com/boostorg/spirit/blob/master/workbench/measure.hpp ... Joel de Guzman

Thank you for the pointers. I've 1) #included "qi/int_parser.cpp"; 2) made spirit_new_test from spirit_int_test; 3) added boost::convert()-based test; 4) compiled with gcc (Ubuntu 4.8.2-19ubuntu1) 4.8.2 with "g++ -O2 -std=c++11 ..." 5) got the following results: atoi_test: 2.2135431510 [s] {checksum: 730759d} strtol_test: 2.1688206260 [s] {checksum: 730759d} spirit_int_test: 0.5207926250 [s] {checksum: 730759d} spirit_new_test: 0.5803735980 [s] {checksum: 730759d} cnv_test: 0.6161884860 [s] {checksum: 730759d} struct spirit_new_test : test::base { static int parse(std::string const& str) { std::string::const_iterator beg = str.begin(); std::string::const_iterator end = str.end(); int n; if (boost::spirit::qi::parse(beg, end, boost::spirit::qi::int_, n)) if (beg == end) return n; return (BOOST_ASSERT(0), 0); } void benchmark() { for (int i = 0; i < 9; ++i) this->val += parse(numbers[i]); } }; struct cnv_test : test::base { void benchmark() { for (int i = 0; i < 9; ++i) this->val += boost::convert<int>(numbers[i], boost::cnv::spirit()).value(); } }; 1. The changes in spirit_new_test compared to the original spirit_int_test were additional conversion validity checks and the std::string... the same way boost::cnv::spirit works. That slowed spirit_new_test some 11% compared to spirit_int_test. Though raw conversion speed is important (spirit_int_test) I do not expect it deployed like that (with no checks) in a realistic setting. 2. boost::cnv::spirit implements the same process-flow as spirit_new_test and boost::cnv::spirit was consistently 6% slower compared to raw spirit conversion deployment with the same functionality (spirit_new_test). Thank you, Joel, for sharing your performance testing framework. -- View this message in context: http://boost.2283326.n4.nabble.com/review-Convert-library-tp4662821p4663981.... Sent from the Boost - Dev mailing list archive at Nabble.com.

Joel de Guzman wrote

On 6/11/14, 2:58 PM, Vladimir Batov wrote:

...

... Thank you, Joel, for sharing your performance testing framework.

My pleasure. I'm glad it helped. BTW, as I said before, you can use the low-level spirit ...

Thanks, very much appreciated. Although Spirit is such a Terra-incognito for me. So, I am hoping still Jeroen will jump in and do that. :-) Now, let me get back to the performance tests... You gave me this new "toy" to play with... so now I cannot stop spamming the list with my new findings. Apologies. Still, it seems important as there were concerns voiced about boost::convert() performance penalties. The results I posted before were for your vanilla performance tests... well, with an addition of 2 of my own tests: atoi_test: 2.2135431510 [s] {checksum: 730759d} strtol_test: 2.1688206260 [s] {checksum: 730759d} spirit_int_test: 0.5207926250 [s] {checksum: 730759d} spirit_new_test: 0.5803735980 [s] {checksum: 730759d} cnv_test: 0.6161884860 [s] {checksum: 730759d} However, I felt somewhat uneasy with the limited number (9) of strings tested. More importantly, I felt that the short strings were too heavily represented in the test. What I mean is, for example, there are only 10 1-digit strings out of enormous sea of available numbers. That is, statistically, they are only 10 * 100% / (INT_MAX * 2) out of all available numbers. However, in the test they contributed to 11% of performance results. And I felt that short strings might be spirit's "speciality" so to speak. In other words, I felt that the test might use the input data that favored spirit. So, I replaced those 9-strings input set with 1000 randomly generated strings from the [INT_MIN, INT_MAX] range... and that's the results I've got: local::strtol_test: 312.5899575630 [s] {checksum: 7aa26f0b} local::old_spirit_test: 132.2640077370 [s] {checksum: 7aa26f0b} local::new_spirit_test: 148.1716253210 [s] {checksum: 7aa26f0b} local::cnv_test: 143.4929925850 [s] {checksum: 7aa26f0b} 1) With the original 9-strings test spirit was 4 times faster than strtol; with 1000 strings the difference is down to about 2.5 times... which is what I've been getting: str-to-int spirit/strtol=1.45/3.76 seconds; 2) the overhead of "new_spirit_test" compared to "old_spirit_test" is still about 12%. What is important is that the only difference between the tests is 2 additional validity checks: struct old_spirit_test : test::base { ... boost::spirit::qi::parse(beg, end, boost::spirit::qi::int_, n); return n; } struct new_spirit_test : test::base { ... if (boost::spirit::qi::parse(beg, end, boost::spirit::qi::int_, n)) if (beg == end) return n; return (BOOST_ASSERT(0), 0); } It seems that Spirit is really testing the speed limits given that other operations start playing visible role... like those (necessary IMO) checks add 12%! 3) The "funny" (as you mentioned before) part is that with 1000-strings set the cnv_test is again better than raw new_spirit_test (which has the same process flow as cnv_test). That's what my tests have been showing all along (although they are run against 10000000-strings input set): str-to-int spirit: raw/cnv=1.45/1.44 seconds (99.05%). str-to-int spirit: raw/cnv=1.45/1.44 seconds (99.00%). str-to-int spirit: raw/cnv=1.45/1.44 seconds (99.01%). str-to-int spirit: raw/cnv=1.45/1.44 seconds (99.07%). str-to-int spirit: raw/cnv=1.45/1.44 seconds (99.59%). All compiled with gcc-4.8.2 I personally have no explanation to that but at least I feel that boost::convert() framework does not result in performance degradation as we were concerned it might be... seems to be the opposite. -- View this message in context: http://boost.2283326.n4.nabble.com/review-Convert-library-tp4662821p4663988.... Sent from the Boost - Dev mailing list archive at Nabble.com.

On Jun 12, 2014, at 2:30 AM, Joel de Guzman wrote:

I do not think a random distribution of number of digits is a good representation of what's happening in the real world. In the real world, especially with human generated numbers(*), shorter strings are of course more common.

A well known real world property is Benford's law, often used in fraud detection to check is numbers are fake or "natural". If you draw random numbers uniformly from the logarithmic scale then you'll get that scale invariant property. I think that leads to a random number of digits? http://en.m.wikipedia.org/wiki/Benford's_law#Mathematical_statement

On 6/12/14, 4:41 PM, Joel de Guzman wrote:

On 6/12/14, 2:45 PM, Thijs (M.A.) van den Berg wrote:

...

On Jun 12, 2014, at 2:30 AM, Joel de Guzman wrote:

...

I do not think a random distribution of number of digits is a good representation of what's happening in the real world. In the real world, especially with human generated numbers(*), shorter strings are of course more common.

A well known real world property is Benford's law, often used in fraud detection to check is numbers are fake or "natural".

If you draw random numbers uniformly from the logarithmic scale then you'll get that scale invariant property. I think that leads to a random number of digits?

http://en.m.wikipedia.org/wiki/Benford's_law#Mathematical_statement

That one is for the first digit only and not for the number of digits. Is it just a conjecture that single digits, for example, occur more frequently than say 1,000,000 digits? If that conjecture does not hold, then we should probably be using big nums all over! It's also a known *fact* that varint encoding gives the best performance compared to uniform encoding when transferring data over networks!

I'm not sure if there's a study of the probability of the occurrence of N digits, is there? Anyway, here's one:

http://mathematicalmulticore.wordpress.com/2011/02/04/which-numbers-are-the-...

Perhaps the math guys should set me straight and I would not be surprised if the answer is 42 again! :-)

Just for fun: google any single or double digit number (e.g. "1") and then google a many digit number (e.g. "2432345676"). For "1", I got 15,550,000,000 hits. For "2432345676", I got 9 hits. Regards, -- Joel de Guzman http://www.ciere.com http://boost-spirit.com http://www.cycfi.com/

Sitmo Consultancy B.V. Financial Modelling & Data Science + 31 6 24110061 thijs@sitmo.com P.O. Box 1059, 2600BB, Delft, The Netherlands On 12 Jun 2014, at 10:41, Joel de Guzman wrote:

On 6/12/14, 2:45 PM, Thijs (M.A.) van den Berg wrote:

...

On Jun 12, 2014, at 2:30 AM, Joel de Guzman wrote:

...

I do not think a random distribution of number of digits is a good representation of what's happening in the real world. In the real world, especially with human generated numbers(*), shorter strings are of course more common.

A well known real world property is Benford's law, often used in fraud detection to check is numbers are fake or "natural".

If you draw random numbers uniformly from the logarithmic scale then you'll get that scale invariant property. I think that leads to a random number of digits?

http://en.m.wikipedia.org/wiki/Benford's_law#Mathematical_statement

That one is for the first digit only and not for the number of digits. Is it just a conjecture that single digits, for example, occur more frequently than say 1,000,000 digits? If that conjecture does not hold, then we should probably be using big nums all over! It's also a known *fact* that varint encoding gives the best performance compared to uniform encoding when transferring data over networks!

I'm not sure if there's a study of the probability of the occurrence of N digits, is there? Anyway, here's one:

http://mathematicalmulticore.wordpress.com/2011/02/04/which-numbers-are-the-...

Perhaps the math guys should set me straight and I would not be surprised if the answer is 42 again! :-)

You’re right. There are indeed many distributions that give rise to Benford’s law. Maybe someone should write a script that scrapes all the numbers in boost source files.

Hi, On 2014-05-25 03:40, Vladimir Batov wrote:

...

The most trivial use of boost::convert, and boost::lexical_cast and

...

boost::coerce simply don't compare in my opinion.

boost::cstringstream_converter cnv; int i = boost::convert <int> ::from("23", cnv).value();

versus

int i = boost::lexical_cast <int> ("23"); int j = boost::coerce::as <int> ("23"); Here we go again... :-) lexical_cast will be haunting me to my grave. :-)

What is your evaluation of the design?/ I am not too fond of the frontend API as is. There are several suggestions for a leaner API in the reviews. I second Julian's opinion

No need for that :-) First of all, I think I would not start each and every section of the documentation with how to do something with lexical_cast... Apart from that: I think the main objection here is the amount of code you have to write in order to get the most simple case: I have a value of type X and want to convert it to a value of type Y. I think it would be relatively simple to get to a much leaner interface. For instance, * convert could have a converter template parameter, defaulting to the stringstream converter * convert::from() could have an overload that just takes the "from" parameter and provides the default-constructed converter itself. * convert could have a conversion operator for the OutType, which does the same as convert::value() Then I would have int i = boost::convert<int>::from("17"); As far as I can see, conversion errors are handled by 1. throwing exception (and letting the developer handle that where and how appropriate) 2. using a default value 3. calling a function (e.g. report the problem and provide a default value or throw an exception) With 1) being the default, I could have overloads for 2) and 3), e.g. int i = boost::convert<int>::from("17", 0); int i = boost::convert<int>::from("17", []{ std::cerr << "conversion problem, using default" << std::endl; return 0;} ); And if I want to use a specific converter that, I could use something like MyConverter cnv{some args}; int i = boost::convert<int>::with(cnv).from("17", 0); Or, if the converter can be default constructed: int i = boost::convert::from("17", 0); In a similar way you could create the functors, for instance to be used by algorithms: auto f = boost::convert<int>::functor(); // no need for the <string> template argument if the call operator // is a template, I'd say Without having looked at the code that, I assume that an API like that would be rather easy to accomplish. I am not sure if my opinion is strong enough to be counted as a review, but here it comes anyway: that it would be beneficial to mention parameters in the order [from, to] instead of [to, from].

What is your evaluation of the implementation?

The few files I looked at seemed ok, but I did not really do a code review.

What is your evaluation of the documentation?

Not being familiar with boost::lexical cast, I had difficulties finding the things which are really about boost::convert (the note in "getting started") does not help... The documentation also leaves a lot to guesswork, IMHO. For instance, the examples suggest, that convert<int>::result has a conversion operator to bool which returns true, if the conversion was successful, and false otherwise. If references to lexical_cast were omitted, there would be enough room for explanations.

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

Did you try to use the library? With what compiler? Did you have any

Personally, I have to admit, that I do not really see myself using the library as is. I would write wrappers around it to get an interface closer to what I want. But then, I have to wonder why I don't use the converters directly? And since the converters are mere wrappers to existing stuff afaict, I would probably use the underlying things directly. So unless the API get's cleaned up in a way that does not make me want to wrap it, I see no real use. problems? No.

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

About 3hours including writing this mail. Feel free not to count this as a full review.

Are you knowledgeable about the problem domain?

I'd say yes, having written a few converters myself, having used quite a few more. And finally: Do you think the library should be accepted as a Boost library? I try to avoid conversions like cats try to avoid water. The library does not make we want to go swimming, so to speak. Yes (since I need to swim sometimes), under the condition that the API and the documentation get cleaned up, especially * parameters to -> from * default parameter for the converter * ability for the user of the library to provide methods to be called in case of conversion error o non-throwing methods would return a default value * Consider convert to be of a value on its own, not just in comparison to lexical_cast. If I need be annoyed by lexical_cast in order to understand the value of convert, there is something wrong... * Actually explain the library instead of just giving examples and letting the reader guess. Regards, Roland

On 05/26/2014 12:59 AM, Thijs (M.A.) van den Berg wrote:

On May 25, 2014, at 2:47 PM, Roland Bock wrote:

...

I think it would be relatively simple to get to a much leaner interface. For instance,

* convert could have a converter template parameter, defaulting to the stringstream converter * convert::from() could have an overload that just takes the "from" parameter and provides the default-constructed converter itself. In C++11 has std::to_string and various flavours of stoul, stoull, stof. To me it makes more sense to use those for conversions between strings and integral types. Not just because of portability, but also because the interface is so simple.

The value of convert to me seems to be more in the context of generic programming for a wide range of types with a uniform interface than for a simple interface for specific types for which there are already simple & standard alternatives.

Indeed. Thank you for summing it up. When *I* try answering it turns into 4-volume "War and Piece" and by the time I finish one half is asleep and the other has left. :-)

Roland Bock-2 wrote

On 2014-05-26 01:25, Vladimir Batov wrote:

...

On 05/26/2014 12:59 AM, Thijs (M.A.) van den Berg wrote:

...

...

...

In C++11 has std::to_string and various flavours of stoul, stoull, stof. To me it makes more sense to use those for conversions between strings and integral types. Not just because of portability, but also because the interface is so simple.

The value of convert to me seems to be more in the context of generic programming for a wide range of types with a uniform interface than for a simple interface for specific types for which there are already simple & standard alternatives.

If the ability to be used in generic programming is the main value, then why are almost all the examples in the documentation string->int?

Well, clearly there are many reasons the docs are far from satisfactory... You are not suggesting I spend weeks and weeks writing a bestseller just to see all that rejected during the review and going to the rubbish bin, right? So, I had to be mindful of that.

And I was not (knowingly) suggesting API changes that reduce applicability for generic programming. In fact the ability to provide a callable to handle the conversion problems is much better suited than returning a default value and testing for it, IMHO. It is probably also performing better than throwing and catching exceptions.

I never thought of that. It sounds potentially interesting. I myself have no experience of such deployment... Might be an overkill as so far "optional" served my purposes well... It'd be easier to see real compilable code with realistic examples... even if it is merely string->int. ;-) Many suggestions might well be legit. I just can't possibly try coding all the suggestions and dealing with and working around all potential issues by myself. People should start contributing code. We might start with namespace boost { template optional<TypeOut> convert(TypeIn const& value_in, Converter const& converter) { optional<TypeOut> result; bool success = converter(value_in, result); return success ? result : optional<TypeOut>(); } template typename boost::conversion::algorithm_helper convert(Converter const& cnv) { return boost::conversion::algorithm_helper(cnv); } } So, for ex., the second convert() signature makes another 1-arg or 2-args with the default, well, difficult. So, suggestions should be backed up by how to actually achieve that in code. IMO. Just a potentially more effective way to move forward. -- View this message in context: http://boost.2283326.n4.nabble.com/review-Convert-library-tp4662821p4662881.... Sent from the Boost - Dev mailing list archive at Nabble.com.

Roland Bock-2 wrote

On 2014-05-26 10:45, Vladimir Batov wrote:

...

.. So, I had to be mindful of that.

Sorry, if I annoyed you. Documenting is horribly hard since you know your motivation and library inside out. On the other hand, you received several pieces of input for documentation that could be changed quickly. Why not change them during the review (easy for me to say, I know)?

First, apologies. For taking it on you. The last 2 weeks have be somewhat exhausting. Yes, the docs are not perfect... although I've been somewhat disappointed as I felt we've been focusing too much on documentation rather than the design and api. Without the latter there is no point in documentation. As for "why not change them during the review" I was explicitly instructed by my review manager not to touch the master branch during review. All the suggestions (code and docs) are going into the port_review branch.

...

...

And I was not (knowingly) suggesting API changes that reduce applicability for generic programming. In fact the ability to provide a callable to handle the conversion problems is much better suited than returning a default value and testing for it, IMHO. It is probably also performing better than throwing and catching exceptions. I never thought of that. It sounds potentially interesting. I myself have no experience of such deployment... Might be an overkill as so far "optional" served my purposes well... It'd be easier to see real compilable code with realistic examples... even if it is merely string->int. ;-) We're using such an interface for looking up elements in a map. If the key is invalid, the function is called.

Very nice, for instance, to log and throw.

Do you think you might post some simplified deployment example without divulging trade secrets and killing me with the details? :-) -- View this message in context: http://boost.2283326.n4.nabble.com/review-Convert-library-tp4662821p4662898.... Sent from the Boost - Dev mailing list archive at Nabble.com.

Roland Bock-2 wrote

Using one of the many suggestions for the syntax, please change to what you end up with:

int t = boost::convert <int> ::from("17", []{ log("could not read noOfThreads"); throw ConfigError(); });

int c = boost::convert <int> ::from("17", []{ log("could not read noOfCores, using default"); return 7; });

That looks cool. We might provide a bunch of callables to cover throwable/non-throwable/default-returning/etc. behavior, right? How about non-throwing failure detection? What should be the signature for that behavioral callable? optional<T> op()()? -- View this message in context: http://boost.2283326.n4.nabble.com/review-Convert-library-tp4662821p4662905.... Sent from the Boost - Dev mailing list archive at Nabble.com.

On 26/05/2014 02:59, quoth Thijs (M.A.) van den Berg:

The value of convert to me seems to be more in the context of generic programming for a wide range of types with a uniform interface than for a simple interface for specific types for which there are already simple & standard alternatives.

This comment in the context of strings made me think of stateful converters. I haven't really looked at the proposed library too closely, but is it supported/expected to have mutable converters? An example of this in another conversion library was one that provided for conversion from std::wstring to const char *. This was handled by having a local instance of a "context" object (possibly analogous to the converter) that internally stored any memory allocated during the course of the conversion, such that the returned pointer remained valid as long as the context object was in scope, and no memory was leaked. Furthermore, the way the templates were defined made it a compile error to try to perform that type of conversion without supplying a context object (whereas converting from std::wstring to std::string could be done with or without one, as it did not require intermediate storage). The same context object could be used in multiple conversions and each would be kept distinct (so no conflicts between returned pointers), but all freed at the end.

On 3/06/2014 01:27, quoth Vladimir Batov:

...

An example of this in another conversion library was one that provided for conversion from std::wstring to const char *. This was handled by having a local instance of a "context" object (possibly analogous to the converter) that internally stored any memory allocated during the course of the conversion, such that the returned pointer remained valid as long as the context object was in scope, and no memory was leaked. [...] Gavin, my humble apologies for answering your post/question that late. Somehow I managed to miss it in the avalanche. Now that I am re-reading the

Gavin Lambert wrote threads, I've come across your post and it made me very excited. You are describing IMO a quite interesting/tricky conversion case that the proposed design (with external converters passed in) handles seemingly with ease. In the proposed design converters are created/configured independently from actual conversion. So, they can store configuration/state. Say, std::sstream-based converter remembers hex/dec/oct/uppercase/locale settings, etc. In the case of std::wstring -> "char const*" that might be as follows:

char buf[...]; my_converter cnv;

optional res = convert(from, cnv(buf));

Above "cnv(buf)" takes "buf" and uses it during conversion and returns pointer to it if successful. Does it sound like the case you describe? Admittedly, that would not probably my deployment choice but to me it seems to demonstrate the flexibility/adaptability of the design.

No, that's different from what I was describing. The other conversion library internally allocated the required memory (such that it could allocate exactly the right amount). To use the above style, the code would look more like this: std::wstring from1(L"some text"), from2(L"other text"); converter cnv; optional res1 = convert(from1, cnv); optional res2 = convert(from2, cnv); // pointer values contained in res1 & res2 remain valid as long // as cnv is in scope, and are freed when cnv is destroyed. The main point being that state held within the conversion object is modified as a result of the conversion (specifically, the allocated memory is added to a linked-list to be freed on destruction); thus the converter itself must be a non-const reference. The other thing that was nice about this was that you could obtain the converter generically -- so you could template the method itself and let it accept not just std::wstring but anything that was convertible to char const*, and the appropriate conversion would be automatically selected. Admittedly auto-selecting converters does get trickier if you start to add custom types and multiple possible string representations into the mix, but presumably over time you'd build up "families" of related converters, and the converter family could itself be a template parameter (similar to std::allocator).

I'm sorry. There is a copy-paste mistake in previous mail:

...

// functor interface std::transfrom(first, last, out, convert<T>(from, cnv))); std::transfrom(first, last, out, convert<T>(from, default_v, cnv)));

should be: // functor interface std::transfrom(first, last, out, convert<T>(cnv))); std::transfrom(first, last, out, convert<T>(default_v, cnv))); regards. -- View this message in context: http://boost.2283326.n4.nabble.com/review-Convert-library-tp4662821p4662846.... Sent from the Boost - Dev mailing list archive at Nabble.com.

1) I do like

T t = convert<T>(from, cnv); // Throws on failure T t = convert<T>(from, default_v, cnv); // Returns 'def' on failure

and, if my memory serves me, that was pretty much review#1 interface (minus converters). What it misses though is handling one use-case -- deterministic non-throwing handling of conversion failure. When #2 returns default_v, we do not know if it is due to a failure or "from" just happened to convert to "default_v".

Using a default value means user knows the default value won't be a valid input or it is used as a fallback or just not cared. If conversion failure must be explicitly catched, other interface should be used.

2) I can see two potential problems with a converter provided by default:

a) it might be tricky separating

convert(TypeIn const&, Converter const& =some_default()); convert(Converter const& =some_default());

how about write another overload: convert(TypeIn const&, Converter const&); convert(TypeIn const&); // using default converter

b) different people might feel quite strongly what converter to be the default... and I'll be on the receiving end of their fury. :-(

A default converter may be used in most of the daily job. It's important for boost lib to offer such convenience. If one wants different converter, he can always make himself one. regards. -- View this message in context: http://boost.2283326.n4.nabble.com/review-Convert-library-tp4662821p4662859.... Sent from the Boost - Dev mailing list archive at Nabble.com.

There is

convert(Converter const&)

It'll probably clash with

convert(TypeIn const&)

that's why I suggested using two names(conv_to and convert) in the first place. The implementation could also be effectively simplified. regards. -- View this message in context: http://boost.2283326.n4.nabble.com/review-Convert-library-tp4662821p4662868.... Sent from the Boost - Dev mailing list archive at Nabble.com.

Vladimir Batov-6 wrote

... try and unify all the input so far into a coherent requirements specs, have it voted and proceed implementing it...

Meant "design specs" rather than "requirements specs"... quite a difference. -- View this message in context: http://boost.2283326.n4.nabble.com/review-Convert-library-tp4662821p4662879.... Sent from the Boost - Dev mailing list archive at Nabble.com.