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.

Sign up for GitHub

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

Jump to bottom

Forking boost::lexical_cast as a new module #4287

Closed
wants to merge 3 commits into from
Closed

Forking boost::lexical_cast as a new module #4287

wants to merge 3 commits into from

Conversation

K-ballo
Copy link
Member

@K-ballo K-ballo commented Dec 25, 2019

No description provided.

@hkaiser hkaiser added this to the 1.5.0 milestone Dec 25, 2019
@hkaiser hkaiser added this to TODO in HPX modularization via automation Dec 25, 2019
@biddisco biddisco mentioned this pull request Dec 25, 2019
Open
hkaiser
hkaiser reviewed Dec 25, 2019
View reviewed changes
libs/lexical_cast/include/hpx/lexical_cast/detail/converter_numeric.hpp
#include <limits>
#include <type_traits>

#include <boost/numeric/conversion/cast.hpp>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this be forked as well?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an entire module, and should be handled separately.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is needed for this test only... Should we remove the test?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good to me

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

...which test is that?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Uhh, my bad - it's not a test.

hkaiser
hkaiser reviewed Dec 25, 2019
View reviewed changes
Copy link
Member

@hkaiser hkaiser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please add checks to inspect deprecating the old #include and the use of boost::lexical_cast?

@K-ballo K-ballo force-pushed the lexical_cast branch 3 times, most recently from 426b091 to eabc189 Compare December 26, 2019 15:33
hkaiser
hkaiser reviewed Dec 26, 2019
View reviewed changes
libs/lexical_cast/examples/variant_to_long_double.cpp
@@ -0,0 +1,51 @@
// Copyright Antony Polukhin, 2013-2019.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should remove this particular test as it introduces a dependency on Boost variant.

hkaiser
hkaiser reviewed Dec 26, 2019
View reviewed changes
libs/lexical_cast/include/hpx/lexical_cast/bad_lexical_cast.hpp

#include <hpx/config.hpp>

#include <boost/throw_exception.hpp>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we use our exception handling, instead?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I don't think we can. The errors module should be a foundational module, but it has quite a few dependencies, some of which in turn depend on this module.

We could always replace it with a naked throw.

hkaiser
hkaiser reviewed Dec 26, 2019
View reviewed changes
libs/lexical_cast/tests/unit/CMakeLists.txt
EXCLUDE_FROM_ALL
NOLIBS
DEPENDENCIES hpx_lexical_cast hpx_testing
hpx_filesystem
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need filesystem as a dependent module? Is that for tests only?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like it, yes. I'll move the dependency to tests.

@K-ballo
Copy link
Member Author

K-ballo commented Dec 26, 2019

It occurs to me that there's too much complexity in lexical_cast that we do not need (even after dropping wide char support, locale support, etc). All we need is a pair of to_string/from_string overloads.

@hkaiser
Copy link
Member

hkaiser commented Dec 26, 2019

It occurs to me that there's too much complexity in lexical_cast that we do not need (even after dropping wide char support, locale support, etc). All we need is a pair of to_string/from_string overloads.

Right, you have replace most uses of boost::lexical_cast with standard facilities. There is not much left that depends on lexical_cast.

@hkaiser
Copy link
Member

hkaiser commented Dec 31, 2019

This is superseded by #4288.

@hkaiser hkaiser closed this Dec 31, 2019
HPX modularization automation moved this from TODO to Done Dec 31, 2019
@hkaiser hkaiser deleted the lexical_cast branch December 31, 2019 13:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hkaiser hkaiser hkaiser left review comments

@msimberg msimberg Awaiting requested review from msimberg

Assignees
No one assigned
Labels
type: compatibility issue type: enhancement
Projects
HPX modularization
  
Done
Milestone
1.5.0
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@K-ballo @hkaiser