Boost C++ Libraries: Ticket #6700: Exceptionless lexical_cast

status changed; resolution set

Antony Polukhin — Sat, 06 Oct 2012 20:17:26 GMT

status new → closed
resolution → wontfix

Currently it is not possible, because I can not provide strong exception guarantee. For example, lexical_cast uses std::locale, which can throw. String constructors, some std::stream operators, user defined ostream and istream operators may also throw.

Sat, 06 Oct 2012 22:29:56 GMT

I'm not asking for a variant that doesn't throw at all, just asking for a variant that doesn't throw itself. If some other code throws, that's ok.

Antony Polukhin — Sun, 07 Oct 2012 19:49:28 GMT

Replying to Olaf van der Spek <olafvdspek@…>:

I'm not asking for a variant that doesn't throw at all, just asking for a variant that doesn't throw itself. If some other code throws, that's ok.

I like the idea of try_lexical_cast, but only if it provides strong exception guarantee. Otherwise it may obfuscate users and is not as useful as function with strong exception guarantee.

Sun, 07 Oct 2012 22:42:49 GMT

I assume that's doable.

ben.maurer@… — Wed, 11 Dec 2013 00:06:08 GMT

Any update on if you'd consider doing this? At Facebook we've seen many instances of programs that call lexical_cast in a try/catch. In cases where the parsing is expected to fail (eg, an argument could be an int or a date) we frequently see extremely poor performance in multithreading. GCC is known to have poor multithreaded exception handling gcc.gnu.org/ml/gcc/2013-05/msg00253.html

Facebook is currently running a bug bounty program for open source bugs, and this is a bug that might be worthy of such a bounty, if is something there is interest in merging.

status changed; resolution deleted

Antony Polukhin — Wed, 11 Dec 2013 05:37:23 GMT

status closed → reopened
resolution wontfix

This issue hit me in my own projects.

I'll create a separate branch at github with try_lexical_cast and ask for mini review at mailing lists when it'll be ready.

ben.maurer@… — Wed, 11 Dec 2013 19:42:21 GMT

Thanks! Facebook is putting a $200 bounty on this via bountysource: www.bountysource.com/issues/1352296

attachment set

Thu, 30 Jan 2014 07:17:03 GMT

attachment → boost_1_55_0_lexical_cast_hpp.patch

Patch to add try_lexical_cast function. Patch is against boost_1_55_0 lexical_cast.hpp.

Thu, 30 Jan 2014 07:21:37 GMT

I added the patch here.

try_lexical_cast: A version which doesn't throw bad_lexical_cast. All other exceptions will be thrown as normal.

Getting rid of exceptions thrown in the callbacks nothrow_overflow_handler and detect_precision_loss would mean rewriting boost::numeric::converter, which seems excessive, and futile. And it would create a dependency on those patches going through for this patch to be accepted. Ergo, I resorted to catching these exceptions to provide non throwing behavior.

All other instances where an instance of bad_lexical_cast could be thrown have been replaced with a new function.

Dropped the deprecated call-by-value fallback version template<typename Target, typename Source> bool bool try_lexical_cast(Target& result, Source arg);

Antony Polukhin — Thu, 30 Jan 2014 08:32:29 GMT

Replying to Troy Korjuslommi <troykor@…>:

I added the patch here.

That's a bad patch. It breaks compilation with exceptions off. It does not add tests for new feature and does not update the docs.

It is also much worse than an already applied to develop branch patch:

try {} catch(...){} is not what was requested by users. boost::numeric::converter was the main problem! Without fixing it a patch could be written in 10 minutes, but that would be a patch for outward appearances, does not really resolving the issue.

Thu, 30 Jan 2014 09:16:33 GMT

I can post proper tests and docs once this patch gets some positive comments. The comments in this ticket seem sufficient for reviewing the patch itself.

The lexical_cast function doesn't compile with -DBOOST_NO_EXCEPTIONS either, and this ticket had no mention of it. I did consider it, but it didn't seem to be an concern here. I guess I was wrong. I will see what needs to be done.

The patch against numeric::converter should be developed independently, as it's a separate package. Once it works, try_lexical_cast can be modified to call it's non throwing function. That's a one line change.

As I see it, the patch solves the issue at hand. Adding the -DBOOST_NO_EXCEPTIONS support and calls to modified numeric::converter can be added once the basic functionality is approved.

Antony Polukhin — Thu, 30 Jan 2014 09:40:38 GMT

Replying to Troy Korjuslommi <troykor@…>:

I can post proper tests and docs once this patch gets some positive comments. The comments in this ticket seem sufficient for reviewing the patch itself.

Once again: the issue is already resolved in developer branch of Boost.

The lexical_cast function doesn't compile with -DBOOST_NO_EXCEPTIONS either, and this ticket had no mention of it. I did consider it, but it didn't seem to be an concern here. I guess I was wrong. I will see what needs to be done.

You are wrong. lexical_cast function does compile with -DBOOST_NO_EXCEPTIONS. See results for lexical_cast_no_exceptions_test. And it compiles for a long time!

The patch against numeric::converter should be developed independently, as it's a separate package. Once it works, try_lexical_cast can be modified to call it's non throwing function. That's a one line change.

As I see it, the patch solves the issue at hand. Adding the -DBOOST_NO_EXCEPTIONS support and calls to modified numeric::converter can be added once the basic functionality is approved.

Once again: the issue is already resolved in developer branch of Boost. It is waiting for regression tests to pass, before merging to release. And it is resolved without breaking existing functionality and without "let's do it wrong for now".

If you want to help, try fixing this issue #8261.

If you want to get some easy money from bountysource, then do the work right! Do the work that is not finished!

Thu, 30 Jan 2014 10:16:03 GMT

Next time you work on something, please mention it here too, and provide a link for others to see your work. Now I wasted my time.

Antony Polukhin — Thu, 30 Jan 2014 10:52:14 GMT

Replying to Troy Korjuslommi <troykor@…>:

Next time you work on something, please mention it here too, and provide a link for others to see your work. Now I wasted my time.

Mention at comment 6 does not work for you?

It's a shame that "getting new knowledge" for you is "wasting time".

Sat, 01 Feb 2014 10:06:34 GMT

I will have some time later today, so I will have a look at the #8261 issue.

I did look for try_lexical_cast under github, but it wasn't there. I assumed you had abandoned the task. I realize now I should have used google and not github's own search function to look for the project, since you had apparently changed your mind and posted it under conversion instead of creating a try_lexical_cast entry.

Because I didn't see the entry, I used the latest version of boost, 1.55.0 as the basis for the changes. This seemed sensible at the time. 1.55.0's lexical_cast does not compile with -DBOOST_NO_EXCEPTIONS, but the development branch does. There's the cause for this miscommunication.

Anyway, no need to be upset. I wasn't trying to step on your toes, just thought I'd contribute something. You have the changes under control. That's great. I will have a look at the problem with #8261 and let's see if I can help there.

status, milestone changed; resolution set

Antony Polukhin — Tue, 04 Mar 2014 12:52:27 GMT

status reopened → closed
resolution → fixed
milestone To Be Determined → Boost 1.56.0

Merged to master branch, after ensuring that test failures are not related to try_lexical_convert changes.