Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Begin to systematically remove direct use of lexical_cast in favour o… #742

Merged
merged 5 commits into from
Jan 19, 2022

Conversation

jzmaddock
Copy link
Collaborator

…f existing abstractions.

@jzmaddock
Copy link
Collaborator Author

@mborland : this began as the "simple" task of revising unchecked_factorial.hpp and trying to remove your lexical_cast workaround to see what failed.... it's grown rather larger into replacing lots of lexical_cast usages in favour of existing abstractions. More soon...

Correct silly typo in unchecked_factorial.hpp.
Remove TEST_STD define in config.hpp as it needlessly breaks the TR1 tests.
Remove lexical_cast.hpp workaround file.
Correct #pragma in tr1.hpp.
Stop referencing boost::lexical_cast even in templates which aren't instantiated.
Fix missing macro definition in tr1.hpp.
Correct include order in some tests so we get consistent definitions for BOOST_HAS_FLOAT128.
@jzmaddock
Copy link
Collaborator Author

One stalled test, merging.

@jzmaddock jzmaddock marked this pull request as ready for review January 18, 2022 17:09
@jzmaddock
Copy link
Collaborator Author

On second thoughts this is a big diff, I'll wait and see if anyone wants to comment.

@ckormanyos
Copy link
Member

ckormanyos commented Jan 18, 2022

this began as the "simple" task of revising unchecked_factorial.hpp and trying to remove your lexical_cast workaround to see what failed.

On second thoughts this is a big diff, I'll wait and see if anyone wants to comment.

For a few days, I've been meaning to address a possibly related issue. At the time of writing this post, the rework of unchecked_factorial.hpp broke my nightly build in ckormanyos/wide-decimal, a repository which serves, among other purposes, to provide an incubator for cpp_dec_float.

Anyway, all my jobs failed because this line prevents my math-bindings from getting unchecked_factorial().

Ordinarily I would just patch my sub-project. And ordinarily-ordinarily I'd write a full Multiprecision backend instead of just bindings.

But the point is --- something changed on develop branch where I run nightlies. So I feel it's worth addressing.

I did not yet see if John's rework here addresses this problem.

Cc: @jzmaddock and @mborland and @NAThompson

@ckormanyos
Copy link
Member

ckormanyos commented Jan 18, 2022

did not yet see if John's rework here addresses this problem.

Sorry about the noise, ... the lexical_cast_fixes branch does, in fact, seem to fix the phenomenon I had addressed above.

As a final comment... does this line serve any purpose. I am torn and confused by it. On the one hand, i think only user-defined types will end up instantiating those templates, none of which will have any meaning with <type_traits>'s battery of std::is_-whatever. On the other hand, i can't really figure out what all types will be instantiated there.

Cc: @jzmaddock and @mborland and @NAThompson.

@jzmaddock
Copy link
Collaborator Author

As a final comment... does this line serve any purpose. I am torn and confused by it. On the one hand, i think only user-defined types will end up instantiating those templates, none of which will have any meaning with <type_traits>'s battery of std::is_-whatever. On the other hand, i can't really figure out what all types will be instantiated there.

Regular floating point types (float, double, long double and __float128) are addressed in other specializations, so yes, this template was intended for multiprecision types only. However, there is a bug report somewhere that if you're foolish enough to instantiate unchecked_factorial<unsigned> then you also end up here. Since you really shouldn't be using integer types with this template (unless we choose to extend support to unbounded multiprecision integer types), thes static_asserts are here to catch this misuse. Unfortunately the previous version was incomplete and had a terrible error message :(

So... I'll push some comments to that effect.

@mborland
Copy link
Member

@jzmaddock This looks good to me. Thanks for removing my workarounds.

@jzmaddock jzmaddock merged commit be7b305 into develop Jan 19, 2022
@NAThompson NAThompson deleted the lexical_cast_fixes branch January 24, 2024 21:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants