lballabio / QuantLib

The QuantLib C++ library
http://quantlib.org
Other
5.24k stars 1.78k forks source link

C++ modernization - unordered_set, optional, etc #1270

Closed sweemer closed 2 years ago

sweemer commented 2 years ago

Looks like there were several rounds of C++ modernization in the 1.22 release to replace Boost classes and functions with their std equivalent.

There are a lot more that can be migrated over such as unordered_set, optional, etc. Is there any reason that these haven't been modernized yet, or is it is simply that nobody has gotten around to it yet?

Some of these were obviously introduced after C++11 so they will require the user to compile with a later language version in order to be migrated, but I think the user should at least have the option of using the standard library whenever possible.

lballabio commented 2 years ago

I'd try to avoid combinatorial explosion of compilation flags to test, so I'm not sure I'd introduce C++17 stuff like optional.

I had overlooked unordered_set, which is in C++11. I'd check if it's used in the implementation or the interface, though. If it's the former and we can avoid breaking client code, we might just switch to the std one instead of allowing both.

(In the long run, I'd start deprecating some boost classes and only support the corresponding std ones, too.)

sweemer commented 2 years ago

Looks like the only public interface that exposes boost::unordered_set is Observer:

        typedef boost::unordered_set<ext::shared_ptr<Observable> > set_type;
        typedef set_type::iterator iterator;

If I make these two lines private then everything builds fine, including the tests and examples, so it would seem that they do not need to be public after all.

Of course some user code might use these public types, but in general internal data structures and iterators probably shouldn't be exposed in the public interface anyway. I suggest we deprecate and then make private these public types. If that sounds fine to you then I can submit a pull request with the changes.

lballabio commented 2 years ago

Yes, I think we can deprecate those types. They are in the return type of Observer::registerWith, but I doubt anyone uses the returned value. And if they do, they can use auto to capture it.

sweemer commented 2 years ago

By the way, is there a plan to just always use the std versions of types in the QuantLib::ext namespace? If people are properly using this namespace (or auto) for declarations in their code then there should be issue with making the switch.

I think we can do something similar with boost::optional - define QuantLib::ext::optional using std::optional for C++17 or greater and boost::optional otherwise. Then we can always use std::optional once the default language version for QuantLib bumps up to 17.

In the long run, it would great to be able to compile the core library without a local Boost installation (not the unit tests because those will depend on a unit testing framework dependency, so it may as well continue to be Boost.Test). Most of the usages of Boost within QuantLib should be straightforward to migrate to std, with the main exception of ublas, which will need a bit more thought.