[review] Convert library
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. -- 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"); 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"); 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, 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. 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. -- What is your evaluation of the implementation? I've only had a quick glance at the code and it still looks fine. The gripe I had two years ago regarding the use of reserved names (include guards prefixed with "__") has been fixed. A few comments though: In the boost::lexical_cast based converter there is a try { } catch (...) { } which I believe to be an overly broad catch as lexical_cast explicitly throws a boost::bad_lexical_cast. If in an unlikely event a std::bad_alloc were to be thrown for example I'd rather not have boost::convert hide it from me. In the std::strtol based converter errno isn't checked, and it must be noted that strtol accepts leading whitespace which may or may not be wanted. I've implemented what I believe to be a correct converter using strtol which checks for the leading whitespace in [0], but it comes with no guarantees as it's quite tricky indeed. The new C++11 wrapper std::stol and friends in [1] may be of use here, but unfortunately they only have a throwing interface. All converters are added to the main boost namespace which is crowded enough as it is -- hi boost::_1 --, and libraries should add their functionality to their own namespaces in my opinion. With the current design it's not possible to add them in the boost::convert namespace but I believe they should be in their own namespace, may I suggest boost::converter. It must be said that the first two comments are minor at best, the last regarding the namespace is more important but potentially quite trivial to fix. -- What is your evaluation of the documentation? I've found it to be very hard to write documentation as the library author as at some point you're so intimately familiar with your library that it's hard to get a feel for what the user needs. Whilst there are some rough edges I think the current documentation is decent enough for it not to pose a problem. Again a few comments: The motivation should probably be addressed first, not near the end. The printf and std::strtol based converters aren't documented, and the converters should probably be grouped into a sub-section along with an explanation of how to write your own. Last of all I'd like to see an examples directory showing the examples from the documentation, you already have a test/encryption.hpp which probably belongs there. -- What is your evaluation of the potential usefulness of the library? In the previous review I've argued the library its functionality should be added to boost::lexical_cast. Whilst I still hold that position for the previous version of the library I believe this is no longer the case for the present version, largely due to the added support for multiple backends. There would be no practical way for lexical_cast to add this, and I believe it to be very useful indeed. -- Did you try to use the library? With what compiler? Did you have any problems? I've tried the library with Clang 3.2 and 3.4, and GCC 4.6 and 4.8 on Mac OS X 10.9.3 and the examples from the documentation using the sstream converter compile and run fine. When I tried the printf_converter something as simple as: #include <boost/convert/api.hpp> #include <boost/convert/converter/printf.hpp> int main(int, char *[]) { return boost::convert<int>::from("23", boost::printf_converter()).value(); } failed to compile. It's missing includes as adding the following to converter/printf.hpp fixes the problem: #include <boost/mpl/find.hpp> #include <boost/mpl/vector.hpp> This probably indicates that you're trying to test too much in a single file, and had the includes included via a different boost library. In the previous review I noticed that boost::convert<int>::from('1', cnv).value() spew a rather long error message. This has since been fixed, the error message is now considerably shorter making it easier -- will it ever be easy with C++ -- to figure out what's going on. I must note here that the error message the latest versions of GCC produced is actually easier to figure out than the one Clang produced. -- How much effort did you put into your evaluation? A glance? A quick reading? In-depth study? Something between a quick reading and an in-depth study, now probably leaning towards the latter. -- Are you knowledgeable about the problem domain? As the author of boost::coerce, which I've admittedly ignored for far too long, I'm familiar with the conversion domain. Consequently I've seen the pitfalls and endured the hardships in designing and implementing such a library, my utmost respect for having it reviewed not once, but twice! 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. With that out of the way I'd like to propose the following if you're 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 focussed on backends primarily backed by std::stringstream and friends whereas I have focussed 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 this is currently a major stumbling block for me. Jeroen [0] https://github.com/VeXocide/coerce/blob/master/libs/coerce/example/backend.c... [1] http://en.cppreference.com/w/cpp/string/basic_string/stol [2] https://github.com/VeXocide/coerce/ [3] http://vexocide.org/coerce/
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? :-) 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. :-(
-- 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).
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);
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.
-- 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? :-)
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. 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.
With that out of the way I'd like to propose the following if you're 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 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. 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? 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. -- View this message in context: http://boost.2283326.n4.nabble.com/review-Convert-library-tp4662821p4662827.... Sent from the Boost - Dev mailing list archive at Nabble.com.
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.
Nelson, Erik - 2 wrote
Vladimir Batov wrote on Saturday, May 24, 2014 9:40 PM
Habraken wrote ... 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, ...
Well, we seem to be playing that echo-chamber game. If so, then "I can't possibly stress more that", if you read Jeroen's post and my reply again, then you'll notice that he provided int i = boost::lexical_cast<int>("23"); int j = boost::coerce::as<int>("23"); as the "benchmark" usage examples. I replied that those usage cases do not exist because in real life they are rather uglier: int i = -1; try { i = boost::lexical_cast<int>(str); } catch (...) {} Then, you step in and say "... but such deployment does exist in my code base... Convert "something" to "something else". If it doesn't work, throw an exception." Don't you describe my deployment example rather than Jeroen's?
... 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.
I dare to ask, did not even bother reading "convert" docs? No one is *forcing* you to do anything. If you *want* an exception, there is nothing stopping you from having it. try { int i = boost::lexical_cast<int>(str); int j = boost::convert<int>(str, cnv).value(); } catch(...) {}
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.
I am not sure how to answer this. Essentially you are saying that int i = boost::lexical_cast<int>(str); int j = boost::convert<int>(str, lex_cast_converter()).value(); is "too complex" and your particular deployment so far did not need any of that additional "complexity". I guess, what you perceive as complexity is the price one has to pay for flexibility and wide applicability -- the qualities that we usually strive to have in libraries. -- View this message in context: http://boost.2283326.n4.nabble.com/review-Convert-library-tp4662821p4662830.... Sent from the Boost - Dev mailing list archive at Nabble.com.
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 <vb.mail.247@gmail.com> 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/
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. :-) Jeroen Habraken wrote
...
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.
Jeroen, seriously, we are serious people writing serious s/w. Should we be discussing children playing with matches here? :-)
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!?
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).
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? :-) -- View this message in context: http://boost.2283326.n4.nabble.com/review-Convert-library-tp4662821p4662835.... Sent from the Boost - Dev mailing list archive at Nabble.com.
On 25 May 2014 14:03, Vladimir Batov <vb.mail.247@gmail.com> 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
On 05/26/2014 12:24 AM, Jeroen Habraken wrote:
On 25 May 2014 14:03, Vladimir Batov <vb.mail.247@gmail.com> wrote:
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.
Sorry, I just can't resist picturing that we are developing domestic gas stoves... Whilst we might be developing/manufacturing essential appliance for food-preparation purposes we should be considerate of those who'll be sticking their heads in and blowing their houses up using our appliances. Uhm, I do not think so. :-) That said, if you have an interface that keeps me (ordinary user) happy and works for those thrill-seekers by blowing their heads off... and the community is happy... I am happy... Let's adapt it...
... 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.
Sure thing. Currently "convert" has two overloads. Can you write all overloads that you are proposing? With defaults and stuff. I suspect some might not survive simply because their signatures clash. As for "defaulting to a converter chosen via a customization point", so far I am OK with it... unless we hit some unexpected implementation issue. I only remember Andrzej's "no globals" view. A global would have to be thread-safe and all.
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
Hartmut, 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. On 05/26/2014 12:37 AM, Hartmut Kaiser wrote:
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
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
Jeroen Habraken wrote throw the better.
Well, no one argues that "the simpler a solution the better" and we all want "simple solutions to complex problems" and "more pay for less work", etc. Unfortunately, it is not always the case, is it? Surely, you are not saying that you won't accept a moderate solution to your complex problem if you can't find a simple one.
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.
Yes, your deployment case (of a library developer) makes sense to me. Unfortunately, my deployment cases (of an application developer) are quite different. In my neck of woods reading invalid configuration parameter and abandoning the ship with an exception is, well, an overkill (put very mildly). What I am driving at is that we have wildly differing deployment scenarios. And IMO a library (generic solution) should be able to cater for them all. The conundrum a conversion-related library in general is inevitably facing (and seemingly cannot solve... as we have none so far) is that every single user comes in and says -- my deployment use-case is dead-simple; why should I deploy that "complicated" API instead? IMO that "small-town" thinking has no place in commercial s/w development where modularization and generic solutions are quite important... even probably at the expense of efficiency, etc. Please do not take it as an insult. It's not directed at you or anyone in particular. It's merely my experience in managing considerable projects with finite budgets and rigid timeframes and revolving staff and real customers breathing down your neck for a reliably working solution.
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.
With all due respect this reminds me of the situation during review#1 when the Spirit team was very vocal and uniform (and did not mince their words) in their rejection of the proposal siting that Spirit was opening up unheard-of horizons in the conversion domain. Feel free to call me a dimwit but I personally find Spirit's syntax arcane and using your words "more verbose than absolutely needed" for my conversion purposes. Three years down the track and we are here with you expressing love for the library (Version: 0.2, Last upload: 2010 October 07) that I cannot possibly responsibly use in our commercial development environment due to lack of penetration, reliable support, active development and adequate (for my purposes) flexibility, extensibility and functional-completeness. And, curiously, it does not seem to handle *my* mundane deployment pattern -- non-throwing failure-detection: optional<T> res = convert<T>(from, cnv); if (!res) { log("Invalid ignored. Proceeding with the default"); res = some_default; }
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.
Yes, I understand. Unfortunately, my experience of deploying lexical_cast and my own conversions tells me that in my case the simple deployment interface does not cut it.
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.
Here I am not sure I immediately see what penalties std::tr2::optional introduces. My impression and analysis of the code tells me otherwise. Special requirements (like high performance) require special solutions. Still, that fact did not kill or diminish in any way the existence of generic libraries. More so, during this review I was pleasantly surprised to see more people with similar to mine needs -- they value std::sstream-based converter the most, i.e. functional richness even if not blindly fast. Is "coerce" is the same league?
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.
There is the "Performance" section in the docs. The current design though is different so that "convert" itself has no performance metrics. It's all defined by the plugged-in converter. So, "convert" can perform as fast as "coerce" :-) when coerce-based converter is plugged-in.
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.
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.
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.
From what I see it should be fairly straightforward to eliminate "convert" api. But then again, one has to walk the walk to judge responsibly... and I am not in optimization business.
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.
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. Regards Hartmut --------------- http://boost-spirit.com http://stellar.cct.lsu.edu
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?...
On 26/05/2014 15:34, quoth Vladimir Batov:
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?...
The behaviour being referred to is that most methods come with two overloads: one that accepts a non-const reference to boost::system::error_code (traditionally boost::system::error_code& ec); this form is guaranteed to return errors via the output parameter instead of throwing. The other overload does not have this parameter, and can throw exceptions -- typically as boost::system::system_error(ec) from an internal call to the other overload. Given that Boost.System is in similar standards flux to boost::optional AFAIK, I'm not sure that this necessarily helps. Personally, I think that optional returns are probably more practically useful; while there are times when it might be nice to differentiate between types of conversion failure (eg. unexpected character, not valid in this locale, too large, etc), in practice it doesn't usually make much difference.
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
On May 26, 2014, at 1:34 PM, "Hartmut Kaiser" <hartmut.kaiser@gmail.com> wrote:
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 }
Boost.Math uses this in eg the distribution. Also: http://www.boost.org/community/generic_programming.html#policy
2014-05-25 16:37 GMT+02:00 Hartmut Kaiser <hartmut.kaiser@gmail.com>:
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
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.
[...]
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. Regards, -- Joel de Guzman http://www.ciere.com http://boost-spirit.com http://www.cycfi.com/
Joel de Guzman wrote
On 5/25/14, 6:34 PM, Jeroen Habraken wrote:
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.
2014-05-26 10:12 GMT+02:00 Vladimir Batov <vb.mail.247@gmail.com>:
Joel de Guzman wrote
On 5/25/14, 6:34 PM, Jeroen Habraken wrote:
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.
Can this be true? Your converter concept returns a (small) bool. The API returns something like optional<T> (likely something significantly bigger than bool). This alone could add to performance hit. Worth measuring, at least. Regards, &rzej
On 26 May 2014 10:12, Vladimir Batov <vb.mail.247@gmail.com> wrote:
Joel de Guzman wrote
On 5/25/14, 6:34 PM, Jeroen Habraken wrote:
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
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? :-) -- View this message in context: http://boost.2283326.n4.nabble.com/review-Convert-library-tp4662821p4662887.... Sent from the Boost - Dev mailing list archive at Nabble.com.
2014-05-26 11:08 GMT+02:00 Vladimir Batov <vb.mail.247@gmail.com>:
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.
Andrzej Krzemienski wrote
2014-05-26 11:08 GMT+02:00 Vladimir Batov <
vb.mail.247@
>:
... 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.
Andrzej, you are correct... and I am not suggesting to change the signature. The change from user-friendly optional<T> res = convert<T>(from, cnv); to convert(optional<T>& res, from, cnv); is done by the compiler as part of optimization/elision. That technique's been around for many years. I won't be surprised if optimization is far trickier than that these days. -- View this message in context: http://boost.2283326.n4.nabble.com/review-Convert-library-tp4662821p4662891.... Sent from the Boost - Dev mailing list archive at Nabble.com.
On 26 May 2014 11:08, Vladimir Batov <vb.mail.247@gmail.com> 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
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? -- View this message in context: http://boost.2283326.n4.nabble.com/review-Convert-library-tp4662821p4662892.... Sent from the Boost - Dev mailing list archive at Nabble.com.
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 <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 } -- 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.
On 26 May 2014 11:47, Vladimir Batov <vb.mail.247@gmail.com> wrote:
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 <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. Jeroen
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.
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. :-) -- View this message in context: http://boost.2283326.n4.nabble.com/review-Convert-library-tp4662821p4662902.... 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/
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. -- View this message in context: http://boost.2283326.n4.nabble.com/review-Convert-library-tp4662821p4662908.... Sent from the Boost - Dev mailing list archive at Nabble.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 26 May 2014 14:54, Roland Bock <rbock@eudoxos.de> wrote:
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 ;-)
You had me laughing as well .. Jeroen
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 26 May 2014 11:43, Vladimir Batov <vb.mail.247@gmail.com> 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?
Sorry, wish I could invest more time in this but the latest and greatest can be found at https://github.com/vexocide/coerce. Jeroen
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.
[...]
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...
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 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.
On Tue, Jun 10, 2014 at 8:58 AM, Vladimir Batov <vb.mail.247@gmail.com> 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.
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.
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
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
Oops. Please disregard my reply below. It was meant for the other pimpl-related thread. My bad. On 06/11/2014 06:33 AM, Vladimir Batov wrote:
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
_______________________________________________ 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 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.
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, thank you for the pointers. Much appreciated. Black art, you say? Any blacker-than-black adjectives available that I might use? On 06/11/2014 09:03 AM, Joel de Guzman wrote:
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, 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.
On 6/11/14, 2:58 PM, Vladimir Batov wrote:
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.
My pleasure. I'm glad it helped. BTW, as I said before, you can use the low-level spirit int and real parsers without going through the compile-time consuming expression template facade. For example, here's how integers are parsed: http://tinyurl.com/mb5k9ql Notice: typedef extract_int<T, Radix, MinDigits, MaxDigits> extract; return extract::call(first, last, attr_); You only need to include: #include <boost/spirit/home/qi/numeric/numeric_utils.hpp> The same for unsigned ints and reals. It's all in: boost/spirit/home/qi/numeric/ unsigned ints: http://tinyurl.com/orl62mc floating pt: http://tinyurl.com/nu8vkhh Rip the code in the 'parse' functions for your own use. Then you don't need to wait 22 billion years for the compiler. I won't be surprised if you can still save a few cycles by going lower level, although the compiler should've optimized the extra scaffolding already. Regards, -- Joel de Guzman http://www.ciere.com http://boost-spirit.com http://www.cycfi.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 6/11/14, 7:55 PM, Vladimir Batov wrote:
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:
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. (* e.g. programming languages, which, you are right to say, is spirit's specialty). BTW, the fact that smaller numbers occur more in real life is taken advantage of some optimized encodings such as Google Protocol Buffers's Base 128 Varints where smaller numbers take a smaller number of bytes. If the distribution was equal, then encodings such as Varints would not make sense. (https://developers.google.com/protocol-buffers/docs/encoding)
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.
Shrug. Obviously, there's something wrong with that picture, but I am not sure what. It may be that what's happening here is that some other overhead(*) outweighs the actual conversion by a large factor at that scale and that is what you are actually seeing. (* E.g. string operations) Regards, -- Joel de Guzman http://www.ciere.com http://boost-spirit.com http://www.cycfi.com/
On Jun 12, 2014, at 2:30 AM, Joel de Guzman <djowel@gmail.com> 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, 2:45 PM, Thijs (M.A.) van den Berg wrote:
On Jun 12, 2014, at 2:30 AM, Joel de Guzman <djowel@gmail.com> 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! :-) Regards, -- Joel de Guzman http://www.ciere.com http://boost-spirit.com http://www.cycfi.com/
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 <djowel@gmail.com> 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/
On 12 Jun 2014, at 11:01, Joel de Guzman <djowel@gmail.com> wrote:
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.
That’s a good idea! With a couple of experiment like that I’ll bet you’ll be able to fit a power law distribution of the nr of hits
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 <djowel@gmail.com> 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 <djowel@gmail.com> 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.
On 06/11/2014 06:55 AM, Andrey Semashev 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
On Wednesday 11 June 2014 06:46:53 Vladimir Batov wrote: 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.
Yes, indeed, Andrey, thank you. What I was referring to was: #define BOOST_TEST(expr) ((expr)? (void)0: ::boost::detail::test_failed_impl(#expr, __FILE__, __LINE__, BOOST_CURRENT_FUNCTION)) #define BOOST_ASSERT(expr) (BOOST_LIKELY(!!(expr)) \ ? ((void)0) \ : ::boost::assertion_failed(#expr, BOOST_CURRENT_FUNCTION, __FILE__, __LINE__)) #define BOOST_LIKELY(x) __builtin_expect(x, 1) That is, BOOST_TEST simply tests (expr) when BOOST_ASSERT tests with !!__builtin_expect(expr, 1). That said, you are absolutely correct that the actual validity test is with !! rather than __builtin_expect as I ignorantly expressed it. Thank you.
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<int, MyConverter>::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 May 25, 2014, at 2:47 PM, Roland Bock <rbock@eudoxos.de> 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.
On 05/26/2014 12:59 AM, Thijs (M.A.) van den Berg wrote:
On May 25, 2014, at 2:47 PM, Roland Bock <rbock@eudoxos.de> 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. :-)
On 2014-05-26 01:25, Vladimir Batov wrote:
On 05/26/2014 12:59 AM, Thijs (M.A.) van den Berg wrote:
On May 25, 2014, at 2:47 PM, Roland Bock <rbock@eudoxos.de> 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. :-)
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? 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. Regards, Roland
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<typename TypeOut, typename TypeIn, typename Converter> 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 TypeOut, typename Converter> typename boost::conversion::algorithm_helper<TypeOut, Converter> convert(Converter const& cnv) { return boost::conversion::algorithm_helper<TypeOut, Converter>(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 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. 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
On 2014-05-26 10:45, Vladimir Batov wrote: the review (easy for me to say, I know)?
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. Regards, Roland
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.
On 2014-05-26 12:34, 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
On 2014-05-26 10:45, Vladimir Batov wrote: the review (easy for me to say, I know)? First, apologies. For taking it on you. No problem :-) 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
Roland Bock-2 wrote 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. OK, I was not aware 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. ;-) 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? :-)
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; }); Regards, Roland
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 2014-05-26 14:37, Vladimir Batov 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
Roland Bock-2 wrote throwable/non-throwable/default-returning/etc. behavior, right? Right :-) How about non-throwing failure detection? What should be the signature for that behavioral callable? optional<T> op()()?
Personally, I would not use that (preferring either default or exception), but yes, I guess so. Regards, Roland
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.
Gavin Lambert wrote
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.
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 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<char const*> res = convert<char const*>(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. -- View this message in context: http://boost.2283326.n4.nabble.com/review-Convert-library-tp4662821p4663246.... Sent from the Boost - Dev mailing list archive at Nabble.com.
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<char const*> res = convert<char const*>(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<std::wstring, char const*> cnv; optional<char const*> res1 = convert<char const*>(from1, cnv); optional<char const*> res2 = convert<char const*>(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 agree the converter concept is a step forward. And it's also a good idea to support both exception and defualt value on failure. The functor interface is useful, too. But as other reviewer mentioned, the convert interface seems less intuitive and lack a default convenient interface. Personally, I'd prefer something like: // cast like interface T t = conv_to<T>(from, cnv); // compare to T t = convert<T>::from(from, cnv).value(); T t = conv_to<T>(from, default_v, cnv); // compare to T t = convert<T>::from(from, cnv).value_or(default_v); // plain interface bool re = convert(from, to, cnv); // functor interface std::transfrom(first, last, out, convert<T>(from, cnv))); std::transfrom(first, last, out, convert<T>(from, default_v, cnv))); ** with defualt interface version, "cnv" will be ignored Using only two names, we can get something simple and intuitive. The implemention can be also effectively simplfied. regards. -- View this message in context: http://boost.2283326.n4.nabble.com/review-Convert-library-tp4662821p4662845.... Sent from the Boost - Dev mailing list archive at Nabble.com.
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.
On 05/26/2014 04:04 AM, feverzsj wrote:
I agree the converter concept is a step forward. And it's also a good idea to support both exception and defualt value on failure. The functor interface is useful, too. But as other reviewer mentioned, the convert interface seems less intuitive and lack a default convenient interface.
Personally, I'd prefer something like:
// cast like interface T t = conv_to<T>(from, cnv); // compare to T t = convert<T>::from(from, cnv).value(); T t = conv_to<T>(from, default_v, cnv); // compare to T t = convert<T>::from(from, cnv).value_or(default_v);
// plain interface bool re = convert(from, to, cnv);
// functor interface std::transfrom(first, last, out, convert<T>(cnv))); std::transfrom(first, last, out, convert<T>(default_v, cnv)));
** with defualt interface version, "cnv" will be ignored
Using only two names, we can get something simple and intuitive. The implemention can be also effectively simplfied.
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". 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()); b) different people might feel quite strongly what converter to be the default... and I'll be on the receiving end of their fury. :-(
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.
On 05/26/2014 12:01 PM, feverzsj wrote:
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.
My problem is that I personally have great distaste for exceptions on mandate ordinary program-flow level. With regard to conversion that'd turn my 10-lines code block into 50-lines monster that I have difficulties reading. So, I handle it with optional<T> res = convert<T>(from, cnv); if (!res) { log("Invalid ignored. Proceeding with the default"); res = some_default; } no exceptions, no default-related non-determinism. Happy. And you want to take it way. :-(
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
There is convert(Converter const&) It'll probably clash with convert(TypeIn const&)
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.
Understood. What I am trying to say is that your preferred default might well be not mine. That said, I'll go with the flow. What the majority decides on.
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.
On 05/26/2014 01:36 PM, feverzsj wrote:
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.
Ah, sorry. I missed that. There've been quite a few suggestions regarding the name in particular, regarding the signature and all. I feel overwhelmed. I feel, no matter what I choose, I'll end up defending it... and I am running out of ammunition. :-) There are only two ways that I can see how to proceed 1) I say "I do it my way" but then it makes it my library rather than Boost library and I am uncomfortable doing that; 2) try and unify all the input so far into a coherent requirements specs, have it voted and proceed implementing it... Oh, there is a third way out of this tight spot -- this proposal gets rejected... and all gets peaceful and quiet again... I love peace and quiet... :-)
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.
participants (15)
-
alex -
Andrey Semashev -
Andrzej Krzemienski -
feverzsj -
Gavin Lambert -
Hartmut Kaiser -
Jeroen Habraken -
Joel de Guzman -
Matus Chochlik -
Nelson, Erik - 2 -
Roland Bock -
Thijs (M.A.) van den Berg -
Thijs van den Berg
-
Vladimir Batov -
Vladimir Batov